fixing MIFOSAC-635 : improving code in LoanSummaryScreen#2588
fixing MIFOSAC-635 : improving code in LoanSummaryScreen#2588gurnoorpannu wants to merge 5 commits intoopenMF:developmentfrom
Conversation
📝 WalkthroughWalkthroughReplaces sealed UI state with a data-driven Changes
Sequence Diagram(s)sequenceDiagram
participant Screen as LoanAccountSummaryScreen
participant VM as LoanAccountSummaryViewModel
participant Repo as LoanAccountSummaryRepository
participant Nav as NavController
Screen->>VM: emit OnRetry / OnMoreInfoClick / OnLoanIdCopied / OnRepayment...
VM->>VM: handleAction(action) -> update state / emit event
VM->>Repo: loadLoanById(loanId)
Repo-->>VM: Success(loan) / Failure(error)
VM->>Screen: emit LoanAccountSummaryEvent (via eventFlow)
Screen->>Nav: handle event (NavigateTo..., NavigateBack, etc.)
Screen-->>Screen: show snackbar when state.showLoanIdCopiedMessage == true
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
298-316: Prevent rendering"null"string in summary cards.When
summaryis null, the safe access operator evaluates to null, and calling.toString()on null produces the literal string"null"in the UI. Use?.toString().orEmpty()for null-safe display.Suggested fix
- infoText = summary?.totalExpectedRepayment.toString(), + infoText = summary?.totalExpectedRepayment?.toString().orEmpty(), ... - infoText = summary?.totalRepayment.toString(), + infoText = summary?.totalRepayment?.toString().orEmpty(), ... - infoText = summary?.totalOutstanding.toString(), + infoText = summary?.totalOutstanding?.toString().orEmpty(),
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt`:
- Line 158: Run the module’s spotlessApply to fix formatting issues and update
the offending lambda/spacing so the code matches project style; specifically, in
LoanAccountSummaryScreen.kt correct the onRetry assignment (the
remember(viewModel) { {
viewModel.trySendAction(LoanAccountSummaryAction.OnRetry) } } expression) and
the other similar lambda/spacing occurrences referenced by Spotless so they
conform to the formatter (keep consistent spacing, indentation, and trailing
commas as the codebase requires) and re-run spotlessApply to verify all
violations are resolved.
- Around line 657-695: The button text logic in LoanActionButton treats
closedObligationsMet as active which yields "Make Repayment" while the button is
disabled; change the when and isEnabled logic so closedObligationsMet maps to
the closed label and does not enable the button: update the when in
LoanActionButton (the buttonText computation) to check status.active == true
first, add an explicit branch for status.closedObligationsMet == true that
returns stringResource(Res.string.feature_loan_closed), and ensure the isEnabled
boolean excludes closedObligationsMet (i.e., only true for active,
pendingApproval, or waitingForDisbursal).
.../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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
300-318: Avoid displaying literal "null" when summary is hidden.
summary?.… .toString()yields"null"whensummaryis absent, which surfaces in the UI. Use a safe fallback string.✅ Suggested fix
- infoText = summary?.totalExpectedRepayment.toString(), + infoText = summary?.totalExpectedRepayment?.toString().orEmpty(), @@ - infoText = summary?.totalRepayment.toString(), + infoText = summary?.totalRepayment?.toString().orEmpty(), @@ - infoText = summary?.totalOutstanding.toString(), + infoText = summary?.totalOutstanding?.toString().orEmpty(),
| .height(DesignToken.sizes.buttonHeightMedium), | ||
| shape = DesignToken.shapes.small, | ||
| onClick = onAction, | ||
| colors = ButtonDefaults.buttonColors(containerColor = MaterialTheme.colorScheme.primary), |
There was a problem hiding this comment.
This line is unnecessary as well. The button by default use the primary color.
.../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
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt`:
- Around line 117-147: The EventsEffect block in LoanAccountSummaryScreen
contains Spotless Kotlin formatting violations caused by inconsistent formatting
around the when branches for LoanAccountSummaryEvent; run the formatter and
apply fixes (e.g., execute ./gradlew :feature:loan:spotlessApply) then reformat
the EventsEffect block in LoanAccountSummaryScreen so that the when clauses and
lambda braces conform to project Spotless rules; ensure LoanAccountSummaryEvent
branches (NavigateBack, NavigateToMoreInfo, NavigateToTransactions,
NavigateToRepaymentSchedule, NavigateToDocuments, NavigateToCharges,
NavigateToApproveLoan, NavigateToDisburseLoan, NavigateToMakeRepayment) are
neatly aligned, no trailing whitespace exists, and commit the resulting changes.
- Around line 706-709: The button label Text composable is using
MaterialTheme.colorScheme.background for its color which can lack contrast on a
primary-colored button; update the Text color to
MaterialTheme.colorScheme.onPrimary so the buttonText contrasts correctly with
primary backgrounds—locate the Text where buttonText is used in
LoanAccountSummaryScreen (the Text call around the button rendering) and replace
the color reference from background to onPrimary.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (2)
356-367: Consider locale-aware date formatting.The hardcoded
DD/MM/YYYYformat may not match all users' locale expectations. Consider using a locale-aware formatter if internationalization is a requirement.♻️ Suggested locale-aware approach
// Alternative using kotlinx-datetime or platform-specific formatters // For now, keeping as-is is acceptable if locale consistency isn't critical
532-557: Consider adding default value formodifierparameter.Compose convention is to provide
Modifier = Modifieras the default. While this is a private composable and currently always receives an explicit modifier, adding a default improves API consistency.♻️ Suggested fix
private fun InfoCard( titleText: String, infoText: String, - modifier: Modifier, + modifier: Modifier = Modifier, ) {
.../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
|
@gurnoorpannu, please don't resolve conversations on your own. |
| } | ||
|
|
||
| MifosScaffold( | ||
| modifier = modifier, |
There was a problem hiding this comment.
remove mifosScaffold, we don't need it
| InfoCard( | ||
| titleText = stringResource(Res.string.feature_loan_total_loan), | ||
| infoText = summary?.totalExpectedRepayment.toString(), | ||
| infoText = summary?.totalExpectedRepayment?.toString() ?: "0.0", |
There was a problem hiding this comment.
null != 0.00
If we are getting null from the server it doesn't mean the amount is 0.0, it simply means data is not available,
Use N/A instead
| InfoCard( | ||
| titleText = stringResource(Res.string.feature_loan_amount_paid), | ||
| infoText = summary?.totalRepayment.toString(), | ||
| infoText = summary?.totalRepayment?.toString() ?: "0.0", |
| InfoCard( | ||
| titleText = stringResource(Res.string.feature_loan_outstanding_balance), | ||
| infoText = summary?.totalOutstanding.toString(), | ||
| infoText = summary?.totalOutstanding?.toString() ?: "0.0", |
| .height(DesignToken.sizes.buttonHeightMedium), | ||
| shape = DesignToken.shapes.small, | ||
| onClick = onAction, | ||
| colors = ButtonDefaults.buttonColors(containerColor = MaterialTheme.colorScheme.primary), |
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt
Show resolved
Hide resolved
|
@itsPronay i implemented the changes requested, can you review them |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt`:
- Around line 161-166: LaunchedEffect watching state.showLoanIdCopiedMessage
currently only dispatches OnMessageShown and doesn't present any UI feedback;
update the effect to show the message (e.g., via a SnackbarHostState or Toast)
using state.loanIdCopiedMessage before calling
onAction(LoanAccountSummaryAction.OnMessageShown). Locate the LaunchedEffect
block in LoanAccountSummaryScreen, obtain or create a SnackbarHostState (or pass
one into the composable), call
snackbarHostState.showSnackbar(state.loanIdCopiedMessage) (or trigger a Toast)
when state.showLoanIdCopiedMessage is true, then invoke
onAction(LoanAccountSummaryAction.OnMessageShown) to clear the flag. Ensure you
reference the existing state.showLoanIdCopiedMessage, state.loanIdCopiedMessage,
LaunchedEffect, and onAction(LoanAccountSummaryAction.OnMessageShown) symbols.
- Around line 319-332: The Canvas in LoanAccountSummaryScreen currently sets
contentDescription = "" which is inaccessible; create a descriptive label (e.g.,
"Loan status: Active", "Loan status: Pending approval", "Loan status: Waiting
for disbursal", or "Loan status: Other") by computing a statusText using the
same status checks used for the circle color (inspect
loanWithAssociations.status.active, pendingApproval, waitingForDisbursal) and
pass that string to the Canvas contentDescription instead of an empty string;
ensure the text is localized if app uses resources and matches the color logic
in the onDraw block.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt`:
- Around line 79-83: loadLoanById currently launches a new collector each call
which can cause overlapping collectors to race; change the collection to cancel
any in‑flight work before starting a new one by either replacing
repository.getLoanById(...).collect { ... } with
repository.getLoanById(...).collectLatest { ... } (preferred) or track and
cancel a Job (e.g., a loanLoadJob property) and cancel it before launching a new
viewModelScope.launch; ensure the rest of the handler that updates
mutableStateFlow (and the DialogState) remains unchanged so only the latest
emission is applied.
- Around line 97-101: The call to getString (a suspend function) is inside
mutableStateFlow.update's non-suspending lambda; compute the error message
before calling mutableStateFlow.update and pass the precomputed String into the
update. Concretely, call
getString(Res.string.feature_loan_unknown_error_occured) into a local val (e.g.,
errorMsg) and then call mutableStateFlow.update { it.copy(dialogState =
LoanAccountSummaryState.DialogState.Error(errorMsg)) } so the update lambda no
longer invokes getString.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
283-290: Consider localizing the date format.The date format
"$day/$month/$year"is hardcoded as DD/MM/YYYY. This may not match user locale expectations (e.g., US uses MM/DD/YYYY). Consider using a localized date formatter.
.../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
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt
Show resolved
Hide resolved
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt
Outdated
Show resolved
Hide resolved
# Conflicts: # feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
gurnoorpannu
force-pushed
the
refactor/improve-loan-account-summary-screen
branch
from
bba628e to
b62727f
Compare
| sendEvent( | ||
| LoanAccountSummaryEvent.NavigateToApproveLoan( | ||
| loanAccountNumber, | ||
| action.loanWithAssociations, |
There was a problem hiding this comment.
here we are relying on the UI to pass the LoanWithAssociationsEntity back to us. I suggest you should change this to retrieve the data directly from the ViewModel's own state. this will avoid stale data issues.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
308-321:⚠️ Potential issue | 🟡 MinorUnsafe cast may throw
ClassCastException.Line 311 uses
it as List<Int>which could throwClassCastExceptionif the actual type differs. Thecatchblock only handlesIndexOutOfBoundsException, leavingClassCastExceptionunhandled.🛡️ Suggested fix
fun getActualDisbursementDateInStringFormat(): String { try { return loanWithAssociations.timeline.actualDisbursementDate?.let { - DateHelper.getDateAsString(it as List<Int>) + (it as? List<*>)?.filterIsInstance<Int>()?.let { dateList -> + DateHelper.getDateAsString(dateList) + } } ?: "" - } catch (exception: IndexOutOfBoundsException) { + } catch (exception: Exception) { scope.launch { snackbarHostState.showSnackbar( message = message, ) } return "" } }
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt`:
- Around line 268-276: Handle the edge case in LoanAccountSummaryScreen where
dialogState == null and state.loanWithAssociations == null by adding a fallback
UI or guard: inside the branch that currently checks null -> {
state.loanWithAssociations?.let { ... } } update the logic in
LoanAccountSummaryScreen so that if state.loanWithAssociations is null you
render a sensible fallback (e.g., a Loading/Empty/Error composable or a message
with retry) instead of nothing, or alternatively ensure the parent state cannot
produce this combination; reference the dialogState and
state.loanWithAssociations checks and the LoanAccountSummaryContent call when
implementing the fallback.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (2)
197-197: Consider localizing the content description.The
contentDescription = "More options"is a hardcoded English string. For accessibility and i18n consistency, consider using a string resource.
741-742: MarkgetButtonTextasprivate.
getButtonTextis public while similar utility functions (getButtonActiveStatus,getInflateLoanSummaryValue) are markedprivate. Since this function is only used internally at line 553 within this file, make itprivatefor consistency and to enforce proper encapsulation of internal implementation details.
| null -> { | ||
| state.loanWithAssociations?.let { loanWithAssociations -> | ||
| LoanAccountSummaryContent( | ||
| loanWithAssociations = loanWithAssociations, | ||
| onAction = onAction, | ||
| snackbarHostState = snackbarHostState, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Edge case: Empty screen when dialogState is null but loanWithAssociations is also null.
If the state enters a condition where dialogState == null and loanWithAssociations == null, the user will see an empty screen with no feedback. Consider adding a fallback UI or ensuring this state combination cannot occur.
🛠️ Suggested safeguard
null -> {
- state.loanWithAssociations?.let { loanWithAssociations ->
+ val loanWithAssociations = state.loanWithAssociations
+ if (loanWithAssociations != null) {
LoanAccountSummaryContent(
loanWithAssociations = loanWithAssociations,
onAction = onAction,
snackbarHostState = snackbarHostState,
)
+ } else {
+ // Fallback - should not normally occur
+ MifosProgressIndicator()
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| null -> { | |
| state.loanWithAssociations?.let { loanWithAssociations -> | |
| LoanAccountSummaryContent( | |
| loanWithAssociations = loanWithAssociations, | |
| onAction = onAction, | |
| snackbarHostState = snackbarHostState, | |
| ) | |
| } | |
| } | |
| null -> { | |
| val loanWithAssociations = state.loanWithAssociations | |
| if (loanWithAssociations != null) { | |
| LoanAccountSummaryContent( | |
| loanWithAssociations = loanWithAssociations, | |
| onAction = onAction, | |
| snackbarHostState = snackbarHostState, | |
| ) | |
| } else { | |
| // Fallback - should not normally occur | |
| MifosProgressIndicator() | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt`
around lines 268 - 276, Handle the edge case in LoanAccountSummaryScreen where
dialogState == null and state.loanWithAssociations == null by adding a fallback
UI or guard: inside the branch that currently checks null -> {
state.loanWithAssociations?.let { ... } } update the logic in
LoanAccountSummaryScreen so that if state.loanWithAssociations is null you
render a sensible fallback (e.g., a Loading/Empty/Error composable or a message
with retry) instead of nothing, or alternatively ensure the parent state cannot
produce this combination; reference the dialogState and
state.loanWithAssociations checks and the LoanAccountSummaryContent call when
implementing the fallback.
|
@gurnoorpannu i see the ticket is tiled 'Open Loan Account Summary in full compose screen' , Are we supposed to get rid of the topbar here? this whole section i mean 
|
@itsPronay I had the same confusion earlier, so I asked Rajan sir. He told me the quality of code in this screen was not at par with other screens so I need to improve the code as it is in other screens . SO I took reference from other similar screens and made changes. |
| when (state.dialogState) { | ||
| is LoanAccountSummaryState.DialogState.Error -> { | ||
| MifosSweetError( | ||
| message = uiState.message, | ||
| onclick = onRetry, | ||
| message = state.dialogState.message, | ||
| onclick = { onAction(LoanAccountSummaryAction.OnRetry) }, | ||
| ) | ||
| } | ||
|
|
||
| is LoanAccountSummaryUiState.ShowLoanById -> { | ||
| val loanWithAssociations = uiState.loanWithAssociations | ||
| LoanAccountSummaryContent( | ||
| loanWithAssociations = loanWithAssociations, | ||
| makeRepayment = { makeRepayment.invoke(loanWithAssociations) }, | ||
| approveLoan = { approveLoan.invoke(loanWithAssociations) }, | ||
| disburseLoan = disburseLoan, | ||
| snackbarHostState = snackbarHostState, | ||
| ) | ||
| LoanAccountSummaryState.DialogState.Loading -> { | ||
| MifosProgressIndicator() |
There was a problem hiding this comment.
Create a separate function for dialogState and put all the dialogue-states there.
|
|
||
| LoanAccountSummaryUiState.ShowProgressbar -> { | ||
| MifosProgressIndicator() | ||
| null -> { |
There was a problem hiding this comment.
LoanAccountSummaryContent should not be inside DialogState's null block, you may see other screens for reference
| } | ||
| } | ||
|
|
||
| private fun getButtonActiveStatus(status: LoanStatusEntity): Boolean { |
| } | ||
|
|
||
| @Composable | ||
| private fun getInflateLoanSummaryValue(status: LoanStatusEntity): Boolean { |
| } | ||
|
|
||
| @Composable | ||
| fun getButtonText(status: LoanStatusEntity): String { |
There was a problem hiding this comment.
These types of functions should be in the viewModel according to the KMP structure that we are following.
By 'these' I meant anything logical/not ui related
| } | ||
|
|
||
| private fun loadLoanById() { | ||
| loadJob?.cancel() |
There was a problem hiding this comment.
why not viewmodelscope, any specific reason why we are using job here!?!
| title = stringResource(Res.string.feature_loan_loan_account_summary), | ||
| onBackPressed = navigateBack, | ||
| onBackPressed = { onAction(LoanAccountSummaryAction.NavigateBack) }, |
There was a problem hiding this comment.
lets get rid of this extra topbar, as we have introduced 'MifosBreadCrumb` already
4 participants
Fixes - MIFOSAC-#635
All Changes implemented:-
Adopted event-based navigation and UI flow
Replaced direct callback-based navigation with a proper LoanAccountSummaryEvent system
Navigation is now handled via EventsEffect, consistent with other feature screens
UI now communicates with ViewModel using a single onAction callback
Improved state handling
Replaced isLoading and error: String? with a structured DialogState sealed interface
Follows the same pattern used in ClientProfileState and SettingsState
Makes loading and error handling more predictable and type-safe
Cleaned up ViewModel responsibilities
Marked LoanAccountSummaryViewModel as internal (consistent with other ViewModels)
Removed public helper methods that were being exposed unnecessarily
Business logic helpers were moved out of the ViewModel or made private
Removed business logic from composables
Functions like date formatting and summary expansion logic were removed from UI lambdas
UI now receives clean, prepared state instead of calling ViewModel helpers directly
This improves testability and separation of concerns
Fixed composable structure and naming
Removed duplicate composables with the same name
Introduced a clear hierarchy:
Screen → Scaffold → Content → Dialog
Matches the structure used by ClientProfileScreen and SearchScreen
Improved action definitions
Removed unused parameters from actions (e.g. OnLoanIdCopied)
Actions now only contain data that is actually used
Minor UI improvements
Added proper top bar handling consistent with other full-screen Compose screens
Ensured the Loan Account Summary behaves as a true full-screen destination
| Before :
2026-01-29.14.27.53.mp4
After:
vid.mp4
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
User Experience
Refactor