feat(loan): navigate to loan list after successful loan creation - #2674
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR implements post-loan creation navigation by threading a clientId callback through the new-loan account feature. It adds a delayed Post-loan creation navigation flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.kt (1)
47-58: 💤 Low valueConfirm intermediate back-stack state during pop+navigate.
The two-step approach (
popBackStackto the existingClientLoanAccountsRoute(clientId)withinclusive = true, followed bynavigate(...)withpopUpTo<NewLoanAccountRoute> { inclusive = true }) achieves a freshClientLoanAccountsRouteinstance and removesNewLoanAccountRoutefrom the back stack. A few things to be aware of:
- When the user reached
NewLoanAccountRoutefromClientLoanAccountsRoute, thepopBackStack(route = ClientLoanAccountsRoute, inclusive = true)already pops bothClientLoanAccountsRouteandNewLoanAccountRoute(everything above the target plus the target). The subsequentpopUpTo<NewLoanAccountRoute>(inclusive = true)then becomes a no-op in that scenario, which is harmless but worth noting.- When the user reached
NewLoanAccountRoutefrom a different entry (e.g.,ClientApplyNewApplication),ClientLoanAccountsRouteisn't on the stack and the firstpopBackStackis a silent no-op; thepopUpTo<NewLoanAccountRoute>then handles removing the new-loan screen. Good.- A simpler equivalent that avoids the two-step pop and the cross-feature import would be a single
navigate(ClientLoanAccountsRoute(clientId)) { popUpTo<NewLoanAccountRoute> { inclusive = true }; launchSingleTop = true }, with the loan-list screen subscribing to a refresh signal (e.g., re-fetch inLaunchedEffect(Unit)on resume / on state observed from the repo). That keeps the navigation reactive and removes feature-module coupling.Please verify on device that there is no visible flicker of the underlying screen between pop and navigate, and that the new
ClientLoanAccountsRouteinstance reliably reloads the latest loans (no stale list).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.kt` around lines 47 - 58, The two-step popBackStack + navigate in navigateToClientLoanAccountsAfterCreation can be simplified and decoupled: replace the popBackStack(...) then navigate(...) sequence with a single navigate(ClientLoanAccountsRoute(clientId)) { popUpTo<NewLoanAccountRoute> { inclusive = true }; launchSingleTop = true } to avoid cross-feature coupling and intermediate back-stack flicker, and ensure ClientLoanAccountsRoute's ViewModel reloads fresh by subscribing to a refresh signal (e.g., trigger re-fetch in ClientLoanAccountsRoute's LaunchedEffect/onResume or observe repo state) so the list updates after navigation.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (1)
243-253: 💤 Low valueVerify behavior if dialog dismisses before the 1s delay completes.
The nested
launch { snackbarHostState.showSnackbar(...) }correctly avoids blocking the post-success flow on the snackbar's dismissal (a real fix). However, since thisLaunchedEffectlives inside theis DialogState.SuccessResponseStatusbranch, ifstate.dialogStateis reset tonull(or any other branch) within that 1-second window, the effect leaves composition, thedelayis cancelled, andNewLoanAccountAction.LoanCreatedis never dispatched — silently aborting navigation.Given the current ViewModel paths I can see, dialogState isn't cleared automatically on success, so in practice this is unlikely to trigger. Still worth confirming there is no UI/flow that resets the dialog during the success window (e.g., user-triggered dismiss, back press handling, retry). If you want to harden this, dispatch
LoanCreateddirectly from the ViewModel'sDataState.Successbranch (after the snackbar message is set) instead of doing the delay+dispatch from the composable, so it isn't tied to dialog-state lifecycle.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt` around lines 243 - 253, The LaunchedEffect inside the NewLoanAccountState.DialogState.SuccessResponseStatus branch can be cancelled if the dialog state leaves composition before the 1s delay completes, preventing onAction(NewLoanAccountAction.LoanCreated) from firing; to fix, move the navigation dispatch out of the composable and into the ViewModel’s DataState.Success handling (set the snackbar message there and after setting it dispatch LoanCreated), or alternatively run a LaunchedEffect keyed to a stable success key (not dialogState) so the delay+onAction(NewLoanAccountAction.LoanCreated) is not tied to the dialog’s compositional lifetime; update references accordingly (NewLoanAccountState.DialogState.SuccessResponseStatus, LaunchedEffect, snackbarHostState.showSnackbar, onAction(NewLoanAccountAction.LoanCreated), ViewModel DataState.Success).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.kt`:
- Around line 47-58: The two-step popBackStack + navigate in
navigateToClientLoanAccountsAfterCreation can be simplified and decoupled:
replace the popBackStack(...) then navigate(...) sequence with a single
navigate(ClientLoanAccountsRoute(clientId)) { popUpTo<NewLoanAccountRoute> {
inclusive = true }; launchSingleTop = true } to avoid cross-feature coupling and
intermediate back-stack flicker, and ensure ClientLoanAccountsRoute's ViewModel
reloads fresh by subscribing to a refresh signal (e.g., trigger re-fetch in
ClientLoanAccountsRoute's LaunchedEffect/onResume or observe repo state) so the
list updates after navigation.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt`:
- Around line 243-253: The LaunchedEffect inside the
NewLoanAccountState.DialogState.SuccessResponseStatus branch can be cancelled if
the dialog state leaves composition before the 1s delay completes, preventing
onAction(NewLoanAccountAction.LoanCreated) from firing; to fix, move the
navigation dispatch out of the composable and into the ViewModel’s
DataState.Success handling (set the snackbar message there and after setting it
dispatch LoanCreated), or alternatively run a LaunchedEffect keyed to a stable
success key (not dialogState) so the
delay+onAction(NewLoanAccountAction.LoanCreated) is not tied to the dialog’s
compositional lifetime; update references accordingly
(NewLoanAccountState.DialogState.SuccessResponseStatus, LaunchedEffect,
snackbarHostState.showSnackbar, onAction(NewLoanAccountAction.LoanCreated),
ViewModel DataState.Success).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a870a54e-2df0-40dc-b4bb-f2cc11e1ac53
📒 Files selected for processing (6)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
b898120 to
d3d2692
Compare
|
@biplub1 reverted the changed that mb I didn't verify the branch I was pushing to |
| onFinish: () -> Unit, | ||
| onNavigateToLoanList: (clientId: Int) -> Unit, |
There was a problem hiding this comment.
There is already an onFinish, why add a new onNavigateToLoanList?
| data object Finish : NewLoanAccountAction | ||
| data object LoanCreated : NewLoanAccountAction |
There was a problem hiding this comment.
There is a Finish as well as a LoanCreated. Could you please explain why?
| if (state.dialogState.successStatus) { | ||
| delay(1000) | ||
| onAction(NewLoanAccountAction.Finish) | ||
| onAction(NewLoanAccountAction.LoanCreated) |
There was a problem hiding this comment.
Could you please tell us why Finish is being replaced with LoanCreated?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt (1)
403-415: ⚡ Quick winCurrency-symbol derivation is locale-fragile.
Trimming trailing digits/
./,/space from the formatter's output assumes the symbol is always a leading prefix. In locales where the currency code/symbol is a suffix (e.g.,0.00 USD,0,00 €),trimEndstops at the first non-matching character from the right, so for0.00 USDthe result is the full0.00 USD(sinceDis not in the predicate set), which would then be rendered as the input-field prefix. Even for$0.00this works only by coincidence of the symbol being non-numeric.Consider exposing a dedicated symbol/prefix accessor on
CurrencyFormatter(e.g.,CurrencyFormatter.symbolFor(code)) and using that here, or precomputing the prefix once on the common layer with a known sentinel and stripping it consistently. As per learnings, also pass a non-null default forcurrencyCode(e.g.,template.currency?.code ?: "$") so all platform targets behave uniformly whencodeis null.Based on learnings: when a Kotlin parameter like
currencyCode: String?is nullable but downstream calls require a non-null value, provide a safe default (e.g.,currencyCode ?: "$") and centralize normalization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt` around lines 403 - 415, The currency-symbol extraction using CurrencyFormatter.format and trimEnd is locale-fragile; replace it by using a dedicated symbol accessor (e.g., CurrencyFormatter.symbolFor(code)) or compute a normalized formatted string with a sentinel and strip that sentinel to reliably extract the prefix instead of trimEnd; ensure you pass a non-null currency code to the extractor (use template.currency?.code ?: "<default>" or a chosen default like "$") so the logic in the currencySymbol computation (the remember block that references template.currency?.code and template.currency?.decimalPlaces) always gets a stable, platform-consistent input and returns a true symbol/prefix rather than a locale-dependent string fragment.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt (1)
335-345: ⚡ Quick winSilently swallowing a null template can present a meaningless form.
template ?: LoanRepaymentTemplateEntity()treats a null payload as success and renders an empty repayment form (zero principal/interest/fees, no payment-type options) which the user may then submit, producing a request whosetransactionAmountderives from zero and whose payment-type list is empty. Treatingnullas a load failure (re-usingTemplateLoadFailedso the retry path is offered) better matches user intent.♻️ Proposed handling for a null template
repository.getLoanRepayTemplate(args.loanId) .onSuccess { template -> - sendAction( - LoanRepaymentAction.Internal.TemplateLoaded( - template = template ?: LoanRepaymentTemplateEntity(), - ), - ) + if (template == null) { + sendAction(LoanRepaymentAction.Internal.TemplateLoadFailed) + } else { + sendAction(LoanRepaymentAction.Internal.TemplateLoaded(template = template)) + } } .onFailure { sendAction(LoanRepaymentAction.Internal.TemplateLoadFailed) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt` around lines 335 - 345, The onSuccess branch of repository.getLoanRepayTemplate currently treats a null template as success by substituting LoanRepaymentTemplateEntity(), which yields an empty form; change the handling so that when the returned template is null you call sendAction(LoanRepaymentAction.Internal.TemplateLoadFailed) instead of sending TemplateLoaded with an empty entity. In other words, in the success callback of getLoanRepayTemplate check if template == null then send TemplateLoadFailed; otherwise send LoanRepaymentAction.Internal.TemplateLoaded(template = template). This keeps TemplateLoadFailed/ retry behavior and avoids presenting a meaningless empty form.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt`:
- Around line 387-394: The failure handler in LoanRepaymentViewModel currently
surfaces raw throwable.message to the user via
LoanRepaymentAction.Internal.SubmitFailure; change it to default to the
localized fallback (getString(Res.string.feature_loan_payment_error_generic))
and only substitute a user-facing message when the exception is explicitly
domain-mapped (e.g., check type or use a map/extension that returns non-null
userMessage). Keep throwable logged for diagnostics (do not display) and ensure
sendAction always sends either the fallback or the sanitized domain-mapped
message to avoid showing raw network/serialization errors.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt`:
- Around line 403-415: The currency-symbol extraction using
CurrencyFormatter.format and trimEnd is locale-fragile; replace it by using a
dedicated symbol accessor (e.g., CurrencyFormatter.symbolFor(code)) or compute a
normalized formatted string with a sentinel and strip that sentinel to reliably
extract the prefix instead of trimEnd; ensure you pass a non-null currency code
to the extractor (use template.currency?.code ?: "<default>" or a chosen default
like "$") so the logic in the currencySymbol computation (the remember block
that references template.currency?.code and template.currency?.decimalPlaces)
always gets a stable, platform-consistent input and returns a true symbol/prefix
rather than a locale-dependent string fragment.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt`:
- Around line 335-345: The onSuccess branch of repository.getLoanRepayTemplate
currently treats a null template as success by substituting
LoanRepaymentTemplateEntity(), which yields an empty form; change the handling
so that when the returned template is null you call
sendAction(LoanRepaymentAction.Internal.TemplateLoadFailed) instead of sending
TemplateLoaded with an empty entity. In other words, in the success callback of
getLoanRepayTemplate check if template == null then send TemplateLoadFailed;
otherwise send LoanRepaymentAction.Internal.TemplateLoaded(template = template).
This keeps TemplateLoadFailed/ retry behavior and avoids presenting a
meaningless empty form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 819999fe-9007-44d2-92f2-7ad15df4dc1a
📒 Files selected for processing (17)
core/common/src/androidMain/kotlin/com/mifos/core/common/utils/CurrencyFormatter.android.ktcore/common/src/desktopMain/kotlin/com/mifos/core/common/utils/CurrencyFormatter.desktop.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanRepaymentRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.ktcore/database/src/androidMain/kotlin/com/mifos/room/MifosDatabase.ktcore/database/src/androidMain/kotlin/com/mifos/room/di/DatabaseModule.android.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/accounts/loans/LoanRepaymentRequestEntity.ktcore/database/src/desktopMain/kotlin/com/mifos/room/MifosDatabase.ktcore/database/src/desktopMain/kotlin/com/mifos/room/di/DatabaseModule.desktop.ktcore/database/src/nativeMain/kotlin/com/mifos/room/MifosDatabase.ktcore/database/src/nativeMain/kotlin/com/mifos/room/di/PlatformSpecificModule.ktfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentUiState.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
💤 Files with no reviewable changes (1)
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentUiState.kt
| .onFailure { throwable -> | ||
| val fallback = getString(Res.string.feature_loan_payment_error_generic) | ||
| sendAction( | ||
| LoanRepaymentAction.Internal.SubmitFailure( | ||
| message = throwable.message ?: fallback, | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Avoid surfacing raw throwable.message in the failure dialog.
throwable.message can be a network-stack/serialization detail (e.g., Failed to connect to /…, Expected ',' but was '}' at path …, vendor SDK internals) that is neither user-friendly nor safe to display. Prefer the localized fallback by default and only use exception text when it has been domain-mapped to a user-facing message upstream.
🛡️ Proposed default-to-fallback behavior
.onFailure { throwable ->
- val fallback = getString(Res.string.feature_loan_payment_error_generic)
- sendAction(
- LoanRepaymentAction.Internal.SubmitFailure(
- message = throwable.message ?: fallback,
- ),
- )
+ sendAction(
+ LoanRepaymentAction.Internal.SubmitFailure(
+ message = getString(Res.string.feature_loan_payment_error_generic),
+ ),
+ )
}📝 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.
| .onFailure { throwable -> | |
| val fallback = getString(Res.string.feature_loan_payment_error_generic) | |
| sendAction( | |
| LoanRepaymentAction.Internal.SubmitFailure( | |
| message = throwable.message ?: fallback, | |
| ), | |
| ) | |
| } | |
| .onFailure { throwable -> | |
| sendAction( | |
| LoanRepaymentAction.Internal.SubmitFailure( | |
| message = getString(Res.string.feature_loan_payment_error_generic), | |
| ), | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt`
around lines 387 - 394, The failure handler in LoanRepaymentViewModel currently
surfaces raw throwable.message to the user via
LoanRepaymentAction.Internal.SubmitFailure; change it to default to the
localized fallback (getString(Res.string.feature_loan_payment_error_generic))
and only substitute a user-facing message when the exception is explicitly
domain-mapped (e.g., check type or use a map/extension that returns non-null
userMessage). Keep throwable logged for diagnostics (do not display) and ensure
sendAction always sends either the fallback or the sanitized domain-mapped
message to avoid showing raw network/serialization errors.
| onMoreInfoClicked = onMoreInfoClicked, | ||
| onDocumentsClicked = navController::navigateToDocumentListScreen, | ||
| onNotesClicked = navController::navigateToNoteScreen, | ||
| onLoanCreated = navController::navigateToClientLoanAccountsAfterCreation, |
There was a problem hiding this comment.
| onLoanCreated = navController::navigateToClientLoanAccountsAfterCreation, | |
| onLoanCreated = navController::navigateToClientLoanAccounts, |
| onFinish = navController::popBackStack, | ||
| onNavigateToLoanList = onLoanCreated, |
There was a problem hiding this comment.
There seems to be onFinish present, why do we need another parameter?
There was a problem hiding this comment.
my bad , fixed it : )
There was a problem hiding this comment.
I asked a question, you should have explained the difference before making any changes. It seems onFinish could be used to handle Cancel event. Unfortunately, the naming suggests it to be a Success event, however, it is not. onFinish should be renamed to onDismissed. Again just a suggestion. Please review and modify according to rest of the codebase.
Also, see address the @coderabbitai suggestions.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how onFinish and onDismissed are used across navigation files
rg -n "onFinish" --type=kotlin -C2
echo "---"
# Check newLoanAccountDestination signature
rg -n "newLoanAccountDestination\|fun NavGraphBuilder.newLoanAccountDestination" --type=kotlin -C5
echo "---"
# Check NewLoanAccountScreen signature
rg -n "fun NewLoanAccountScreen" --type=kotlin -C5
echo "---"
# Check how onNavigateBack vs onFinish are used in other destination functions
rg -n "onNavigateBack\|onDismissed\|onCancelled" --type=kotlin -C2 | head -80
Length of output: 14369
@biplab1, great observation — the naming concern is valid when looking at the diff, but after checking the codebase patterns, here's the full picture:
onFinish is a well-established success callback across the codebase, not a cancel callback:
| File | onFinish triggered by |
Mapped to |
|---|---|---|
SavingsAccountScreen.kt |
SavingsAccountEvent.Finish |
navController::popBackStack (after success) |
RecurringAccountScreen.kt |
RecurringAccountEvent.Finish |
{} (no-op, after success) |
CreateShareAccountScreen.kt |
CreateShareAccountEvent.Finish |
{} (no-op, after success) |
NewLoanAccountScreen.kt (this PR) |
NewLoanAccountEvent.Finish(clientId) |
onLoanCreated (after success) |
The cancel/back action is consistently named onNavigateBack (mapped to navController::popBackStack) throughout all screens.
The confusion arose from the diff's intermediate state, where onFinish = navController::popBackStack appeared alongside the new onNavigateToLoanList = onLoanCreated — making onFinish look like a dismiss action. After gurnoorpannu's fix, the current state is:
newLoanAccountDestination(
onNavigateBack = navController::popBackStack, // cancel/back
onFinish = onLoanCreated, // success → navigate to loan list
navController = navController,
)This is consistent with the rest of the codebase. Renaming to onDismissed would diverge from the established onNavigateBack / onFinish convention used across Savings, RecurringDeposit, and ShareAccount features.
Regarding addressing my earlier review suggestions — could you point me to the specific inline comment(s) you'd like actioned? I'd be happy to help.
(^ㅅ^)
✏️ Learnings added
Learnt from: biplab1
Repo: openMF/mifos-x-field-officer-app PR: 2674
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt:0-0
Timestamp: 2026-05-07T03:15:52.671Z
Learning: In `feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt` (Kotlin/Compose Multiplatform), `onFinish: (clientId: Int) -> Unit` is a SUCCESS callback (post-loan-creation), consistent with the project-wide pattern where `onFinish` = successful completion and `onNavigateBack` = cancel/back. Do not flag `onFinish` as a misleading name for a cancel/dismiss action in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: sahilshivekar
Repo: openMF/android-client PR: 2603
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt:43-106
Timestamp: 2026-02-06T13:15:16.968Z
Learning: Guideline: When a Kotlin function parameter is nullable (e.g., balance: Double?, currencyCode: String?, maximumFractionDigits: Int?) and downstream calls require a non-null value, add null-safety handling in all implementations. Specifically, avoid calling Currency.getInstance(currencyCode) with a possibly null currencyCode; provide a safe default (e.g., currencyCode ?: "$") or validate before use. Ensure all platform targets (Android/Desktop/Native) follow consistent null handling, and consider centralizing currencyCode normalization in the common layer if feasible. Add tests covering null currencyCode to prevent NPEs.
Learnt from: sahilshivekar
Repo: openMF/mifos-x-field-officer-app PR: 2647
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt:96-96
Timestamp: 2026-03-16T11:58:32.496Z
Learning: In feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt (Kotlin/Compose Multiplatform), navigateToDashboard uses a direct method reference navController::navigateToLoanDashboardScreen without an ID guard. This is intentional: LoanAccountProfileScreen always provides a valid loanId, and LoanDashboardScreen handles invalid/missing data by showing a 'Failed to fetch Loan details' error state. Do not flag the absence of an ID guard here in future reviews.
Learnt from: kartikey004
Repo: openMF/mifos-x-field-officer-app PR: 2659
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanDisbursement/LoanAccountDisbursementScreen.kt:190-195
Timestamp: 2026-04-01T05:03:14.323Z
Learning: In this repo, existing `SelectableDates` / `SelectableDates.isSelectableDate(utcTimeMillis: Long)` implementations use `Clock.System.now().toEpochMilliseconds()` (UTC epoch millis) for date-boundary checks. During PR reviews, do not flag these checks for not using a timezone-aware `LocalDate` start-of-day approach; treat it as an established project-wide pattern. If a change is desired, handle it as a coordinated project-wide improvement rather than as a per-PR review issue.
There was a problem hiding this comment.
@biplab1 I initially had two callbacks , one was for success and second was for going back . And the navigation of user depends on this . So then I thought yo wanted me to delete the 2 callbacks and want me to merge the logic into one, so I merged the logic and extended Event.Finish to carry clientId.
Now if I can understand you correctly you want to me to name it to OnDismissed,
this is fine I would suggest to name it to something like onLoanCreated, I mean OnDismissed sounds like cancel, but our callback fires on success. Happy to go with whichever you prefer — let me know
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Please take a look at other modules how the naming and navigation is handled and implement accordingly.
Ref: #2674 (comment)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt`:
- Around line 348-349: The current code emits NewLoanAccountEvent.Finish for
both generic close/back and successful create paths, causing non-submit finishes
to be treated as a successful loan creation; change the submit flow to emit a
distinct success event (e.g., NewLoanAccountEvent.LoanCreated(clientId)) from
submitLoanApplication() only, and keep handleFinish() and the fallback
delay/sendEvent call emitting NewLoanAccountEvent.Finish as the plain close
event; then update NewLoanAccountScreen to handle LoanCreated with the existing
(clientId: Int) -> Unit callback while leaving Finish mapped to the normal
close/back behavior (apply the same change patterns where similar Finish
emissions occur around the other noted regions).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c43d86c1-3ae6-47dc-a200-3d37b6bc165b
📒 Files selected for processing (6)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
| navController: NavController, | ||
| onNavigateBack: () -> Unit, | ||
| onFinish: () -> Unit, | ||
| onFinish: (clientId: Int) -> Unit, |
There was a problem hiding this comment.
use proper naming conventions for lambda, property etc e.g onLoanCreated etc
| this.navigate(ClientLoanAccountsRoute(clientId = clientId)) | ||
| } | ||
|
|
||
| fun NavController.navigateToClientLoanAccounts(clientId: Int) { |
There was a problem hiding this comment.
here's the corrected one
fun NavController.navigateToClientLoanAccounts(clientId: Int) {
navigate(ClientLoanAccountsRoute(clientId = clientId)) {
popUpTo<NewLoanAccountRoute> { inclusive = true }
launchSingleTop = true
}
}
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (1)
1125-1129: ⚡ Quick win
LoanCreationSuccessshould carryclientIdto be a self-contained event.
data object LoanCreationSuccessforces the consuming composable to reach back intostate.clientIdrather than getting the value directly from the event. WhileclientIdis immutable in this flow, the proper MVI pattern has events carry all the data they need — avoiding temporal coupling between the event emission and the state read.♻️ Suggested refactor
- data object LoanCreationSuccess : NewLoanAccountEvent + data class LoanCreationSuccess(val clientId: Int) : NewLoanAccountEventIn
submitLoanApplication():- sendEvent(NewLoanAccountEvent.LoanCreationSuccess) + sendEvent(NewLoanAccountEvent.LoanCreationSuccess(state.clientId))In
NewLoanAccountScreen.kt:- NewLoanAccountEvent.LoanCreationSuccess -> onLoanCreated(state.clientId) + is NewLoanAccountEvent.LoanCreationSuccess -> onLoanCreated(event.clientId)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt` around lines 1125 - 1129, Change the event so LoanCreationSuccess carries the clientId rather than being a parameterless data object: replace data object LoanCreationSuccess with a data class (or data object with a property) that includes clientId, update the emission site in submitLoanApplication() to emit NewLoanAccountEvent.LoanCreationSuccess(clientId) (use the current state.clientId there), and update the consumer in NewLoanAccountScreen.kt to read the clientId from the event payload instead of reaching back into state; ensure the NewLoanAccountEvent sealed interface and all match/case branches are updated to the new variant name/signature.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.kt (1)
15-15: ⚡ Quick win
ClientLoanAccountsRoute.ktnow carries a direct dependency onNewLoanAccountRoutefrom the loan feature.
popUpTo<NewLoanAccountRoute>means this route file infeature/clientmust import an internal route fromfeature/loan. If the loan flow's entry route is ever renamed or moved,ClientLoanAccountsRoute.ktwill break silently at runtime.An alternative that avoids touching this file's domain: move the back-stack logic into the caller that already knows about both routes.
♻️ Alternative: keep the popUpTo logic in the navigation wiring layer
Remove the new helper from
ClientLoanAccountsRoute.ktand inline the behavior inClientNavigation.kt, where both route types are already imported:// ClientLoanAccountsRoute.kt — remove the new function entirely -fun NavController.navigateToClientLoanAccounts(clientId: Int) { - navigate(ClientLoanAccountsRoute(clientId = clientId)) { - popUpTo<NewLoanAccountRoute> { inclusive = true } - launchSingleTop = true - } -}// ClientNavigation.kt loanDestination( navController = navController, onMoreInfoClicked = onMoreInfoClicked, onDocumentsClicked = navController::navigateToDocumentListScreen, onNotesClicked = navController::navigateToNoteScreen, - onLoanCreated = navController::navigateToClientLoanAccounts, + onLoanCreated = { clientId -> + navController.navigate(ClientLoanAccountsRoute(clientId = clientId)) { + popUpTo<NewLoanAccountRoute> { inclusive = true } + launchSingleTop = true + } + }, )This keeps each feature's route file free of the other feature's routing internals.
Also applies to: 47-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.kt` at line 15, ClientLoanAccountsRoute.kt currently imports and references NewLoanAccountRoute (via popUpTo<NewLoanAccountRoute>), creating a brittle cross-feature dependency; remove the NewLoanAccountRoute import and any popUpTo<NewLoanAccountRoute> usage from ClientLoanAccountsRoute.kt and revert the route helper there to a feature-local API. Move the back-stack/popUpTo logic into the navigation wiring (ClientNavigation.kt) where both ClientLoanAccountsRoute and NewLoanAccountRoute are already known—implement the popUpTo call (or equivalent back-stack clearing) in the caller inside ClientNavigation.kt so ClientLoanAccountsRoute no longer depends on NewLoanAccountRoute. Ensure symbolic references: update usages around popUpTo, ClientLoanAccountsRoute, NewLoanAccountRoute, and ClientNavigation to reflect this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.kt`:
- Line 15: ClientLoanAccountsRoute.kt currently imports and references
NewLoanAccountRoute (via popUpTo<NewLoanAccountRoute>), creating a brittle
cross-feature dependency; remove the NewLoanAccountRoute import and any
popUpTo<NewLoanAccountRoute> usage from ClientLoanAccountsRoute.kt and revert
the route helper there to a feature-local API. Move the back-stack/popUpTo logic
into the navigation wiring (ClientNavigation.kt) where both
ClientLoanAccountsRoute and NewLoanAccountRoute are already known—implement the
popUpTo call (or equivalent back-stack clearing) in the caller inside
ClientNavigation.kt so ClientLoanAccountsRoute no longer depends on
NewLoanAccountRoute. Ensure symbolic references: update usages around popUpTo,
ClientLoanAccountsRoute, NewLoanAccountRoute, and ClientNavigation to reflect
this change.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt`:
- Around line 1125-1129: Change the event so LoanCreationSuccess carries the
clientId rather than being a parameterless data object: replace data object
LoanCreationSuccess with a data class (or data object with a property) that
includes clientId, update the emission site in submitLoanApplication() to emit
NewLoanAccountEvent.LoanCreationSuccess(clientId) (use the current
state.clientId there), and update the consumer in NewLoanAccountScreen.kt to
read the clientId from the event payload instead of reaching back into state;
ensure the NewLoanAccountEvent sealed interface and all match/case branches are
updated to the new variant name/signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e116f1c-fc1a-4a92-a076-05790e17730e
📒 Files selected for processing (6)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: Additional Code Review Guidelines:
- Null Safety & Stability
- Avoid using
!!operator- Handle null cases explicitly using safe calls or proper state handling
- Do not assume values are always non-null without guarantees
- Architecture Boundaries
- ViewModel must not depend on specific network/library implementations
- Ensure proper separation between data, domain, and presentation layers
- Do not format data (currency, dates, calculations) inside the UI layer
- All formatting must be handled in the ViewModel and exposed via state (e.g., StateFlow)
- Performance Considerations
- Avoid unnecessary recompositions in Compose
- Do not attach heavy logic to frequently changing states (e.g., scrollState)
- Prefer lifting state up instead of recomputing in child composables
- Compose & Navigation Best Practices
- NEVER trigger navigation functions or side-effects directly during composition
- Always wrap navigation calls inside
LaunchedEffectorEventsEffectto avoid repeated execution on recomposition- Avoid triggering intensive side-effects during recomposition
- Navigation routes must be type-safe.
- Ensure all route classes or objects used for navigation are annotated with
@Serializable.
- UI Structure
- Dialogs must be separated into their own composables
- Do not embed dialogs inline within complex main screens
- Localization Consistency
- Ensure all supported languages are updated consistently across modules
- Verify translations exist for newly added UI strings
- Code Cleanliness
- Avoid unnecessary inline comments unless critical
- Remove leftover debug or commented code
- Focus on correctness, readability, and maintainability over cosmetic nitpicks.
- Avoid reviewing README, config, or asset files.
- Prioritize identifying bugs, performance issues, and architectural concerns.
- Naming & Intent Rules:
- Follow the official Kotlin Coding Conventions:
https://kotlinlang.org/docs/coding-conventions.html- Use self-explanatory ...
Files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.kt
**/*ViewModel.kt
⚙️ CodeRabbit configuration file
**/*ViewModel.kt: MVI architecture rules:
- All new features must follow MVI
- ViewModel must extend BaseViewModel
- The ViewModel MUST maintain a single UI state (e.g., a single StateFlow)
instead of multiple separate state variables.Required:
- Use *State, *Event, *Action
- Naming must be consistent (FeatureViewModel, FeatureState, FeatureEvent, FeatureAction)
- Follow unidirectional flow:
Action → ViewModel → State → UIInternal reducer/action architecture rules:
Async operations MUST NOT directly mutate UI state repeatedly inside
Flow collectors, suspend callbacks, or repository result handlers.Repository/network/database results MUST be converted into internal
actions usingsendAction(...).
handleAction(...)must act as the primary reducer responsible for:
- state mutation
- reducer-style state transitions
- triggering follow-up actions
Large async methods must be split into:
- async collection layer
- internal action dispatching
- reducer/state handling
Avoid directly calling another business/data-loading method from
repository collectors or async callbacks.
Prefer dispatching follow-up internal actions instead.Preferred pattern:
repository result
-> sendAction(...)
-> handleAction(...)
-> mutableStateFlow.update { ... }Anti-pattern examples:
mutableStateFlow.update { ... } inside collect { }
fetchX() -> directly calls fetchY() inside async collector
large methods mixing:
- collection
- state mutation
- navigation
- business chainingFlag:
- Multiple
mutableStateFlow.update {}calls inside collect { }- Direct state mutation inside async repository callbacks
- Async methods performing both collection and reducer logic
- Direct business-flow chaining from async collectors
- Missing internal reducer actions for async results
- Missing *State / *Event / *Action
- ViewModel not...
Files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
**/*Screen.kt
⚙️ CodeRabbit configuration file
**/*Screen.kt: Screen architecture rules:Each screen must follow a 2-layer structure:
Layer 1 (Entry/Route Composable):
- Function Name: Typically
*ScreenRouteor the entry-point composable.- Responsibilities: Inject ViewModel (e.g.,
koinViewModel), take navigation lambdas, collect theStateFlow, and handle ViewModel events.- Logic: Should only handle ViewModel interaction, state collection, and triggering navigation in response to ViewModel events.
Layer 2 (Stateful/Content Composable —
*Screenor*ScreenContent):
- Parameters: MUST only take
state(the UI state object) and a singleonActionlambda (e.g.,onAction: (FeatureAction) -> Unit).- Responsibilities: Render the UI based strictly on the provided
state.- Rule: MUST NOT pass multiple separate lambda functions for different UI interactions; consolidate them into the single
onAction.- Constraint: Must NOT contain any business logic or ViewModel/Navigation references.
Internal/private helper composables (e.g., dialogs, sections, sub-components):
- These are NOT subject to the single
onActionrule.- They may accept specific, focused lambdas (e.g.,
onRetry: () -> Unit) or a narrowedonActionas appropriate.- However, they must NOT be passed the ViewModel or navigation controllers directly.
UI consistency:
- Avoid hardcoded values (dp, sp, padding, fontSize, colors)
- Use DesignToken, KptTheme, AppColors, MifosTypography for spacing, typography, and colors
Code quality:
- Keep Composables small and readable
- Avoid deeply nested UI
Flag:
- Missing Layer 2 (
*Screen/*ScreenContent) separation from the entry-point composable- Layer 2 (
*Screen/*ScreenContent) receiving multiple separate lambdas instead of a singleonAction- UI logic inside the entry-point composable
- Business logic inside any Composable
- Hardcoded strings instead of using string resources
- Hardcoded dp/sp values
- Direct styling instead of using DesignToken or KptTheme
Files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt
🧠 Learnings (4)
📚 Learning: 2026-02-06T13:15:16.968Z
Learnt from: sahilshivekar
Repo: openMF/android-client PR: 2603
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt:43-106
Timestamp: 2026-02-06T13:15:16.968Z
Learning: Guideline: When a Kotlin function parameter is nullable (e.g., balance: Double?, currencyCode: String?, maximumFractionDigits: Int?) and downstream calls require a non-null value, add null-safety handling in all implementations. Specifically, avoid calling Currency.getInstance(currencyCode) with a possibly null currencyCode; provide a safe default (e.g., currencyCode ?: "$") or validate before use. Ensure all platform targets (Android/Desktop/Native) follow consistent null handling, and consider centralizing currencyCode normalization in the common layer if feasible. Add tests covering null currencyCode to prevent NPEs.
Applied to files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.kt
📚 Learning: 2026-04-01T05:03:14.323Z
Learnt from: kartikey004
Repo: openMF/mifos-x-field-officer-app PR: 2659
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanDisbursement/LoanAccountDisbursementScreen.kt:190-195
Timestamp: 2026-04-01T05:03:14.323Z
Learning: In this repo, existing `SelectableDates` / `SelectableDates.isSelectableDate(utcTimeMillis: Long)` implementations use `Clock.System.now().toEpochMilliseconds()` (UTC epoch millis) for date-boundary checks. During PR reviews, do not flag these checks for not using a timezone-aware `LocalDate` start-of-day approach; treat it as an established project-wide pattern. If a change is desired, handle it as a coordinated project-wide improvement rather than as a per-PR review issue.
Applied to files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.kt
📚 Learning: 2026-03-16T11:58:32.496Z
Learnt from: sahilshivekar
Repo: openMF/mifos-x-field-officer-app PR: 2647
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt:96-96
Timestamp: 2026-03-16T11:58:32.496Z
Learning: In feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt (Kotlin/Compose Multiplatform), navigateToDashboard uses a direct method reference navController::navigateToLoanDashboardScreen without an ID guard. This is intentional: LoanAccountProfileScreen always provides a valid loanId, and LoanDashboardScreen handles invalid/missing data by showing a 'Failed to fetch Loan details' error state. Do not flag the absence of an ID guard here in future reviews.
Applied to files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
📚 Learning: 2026-02-16T08:37:28.351Z
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersList/ClientIdentifiersListScreen.kt:60-60
Timestamp: 2026-02-16T08:37:28.351Z
Learning: In Kotlin files under the android-client module, replace MaterialTheme references with the established KptTheme import: import template.core.base.designsystem.theme.KptTheme. This should be applied consistently across files where KptTheme is used, replacing any MaterialTheme imports with the correct KptTheme import path.
Applied to files:
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt
🔇 Additional comments (4)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (1)
97-103: LGTM — event handling is cleanly placed inEventsEffect.The
LoanCreationSuccessevent is consumed inEventsEffect(not during composition), and theFinishevent is correctly kept for the non-submit path. The previous ambiguity between the two paths is resolved.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.kt (1)
23-37: LGTM — clean parameter forwarding.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt (1)
77-82: LGTM —onFinishandonLoanCreatedare now cleanly separated.
onFinish = navController::popBackStackhandles the wizard close / last-step path, whileonLoanCreatedis the dedicated post-creation success callback. This resolves the ambiguity flagged in the earlier review.feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
375-381: LGTM — theonLoanCreatedcallback is correctly wired here.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
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/newLoanAccount/NewLoanAccountViewModel.kt (1)
305-353: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftRefactor to follow the MVI internal reducer/action architecture.
The
submitLoanApplicationmethod mixes async collection with state mutation, delay, and event emission inside thecollect {}block. Per coding guidelines, repository results must be converted into internal actions usingsendAction(...), andhandleAction(...)should act as the primary reducer for state transitions.Current anti-pattern:
loanUseCase(payload).collect { dataState -> when (dataState) { is DataState.Success -> { mutableStateFlow.update { ... } // ❌ Direct state mutation in collector delay(1000) // ❌ Business logic in collector sendEvent(...) // ❌ Navigation decision in collector } } }Preferred pattern:
// In submitLoanApplication: loanUseCase(payload).collect { dataState -> when (dataState) { is DataState.Success -> sendAction(NewLoanAccountAction.Internal.LoanCreated) is DataState.Error -> sendAction(NewLoanAccountAction.Internal.LoanCreationFailed(dataState)) DataState.Loading -> sendAction(NewLoanAccountAction.Internal.LoanCreationLoading) } } // In handleAction: is NewLoanAccountAction.Internal.LoanCreated -> { mutableStateFlow.update { it.copy( isOverLayLoadingActive = false, launchEffectKey = Random.nextInt(), dialogState = NewLoanAccountState.DialogState.SuccessResponseStatus( successStatus = true, msg = getString(Res.string.feature_loan_account_created_successfully), ), ) } viewModelScope.launch { delay(1000) sendEvent(NewLoanAccountEvent.LoanCreationSuccess(state.clientId)) } }This separates collection (data layer) from reduction (state management) and keeps business flow decisions in the reducer.
As per coding guidelines: "Repository/network/database results MUST be converted into internal actions using
sendAction(...)" and "handleAction(...)must act as the primary reducer responsible for state mutation, reducer-style state transitions, triggering follow-up actions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt` around lines 305 - 353, The submitLoanApplication collector currently mutates state, performs delay, and emits events inside loanUseCase(payload).collect { ... }—refactor it so the collector only maps DataState to internal actions by calling sendAction(...) (e.g., sendAction(NewLoanAccountAction.Internal.LoanCreated), LoanCreationFailed(dataState), LoanCreationLoading) and remove direct mutableStateFlow updates, delay, and sendEvent calls from the collector; then implement handling for those internal actions in handleAction (handle NewLoanAccountAction.Internal.LoanCreated by updating mutableStateFlow, launching a coroutine to delay(1000) and then sendEvent(NewLoanAccountEvent.LoanCreationSuccess(state.clientId)), and handle error/loading variants similarly) so all state mutations and business flow live in the reducer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt`:
- Around line 305-353: The submitLoanApplication collector currently mutates
state, performs delay, and emits events inside loanUseCase(payload).collect {
... }—refactor it so the collector only maps DataState to internal actions by
calling sendAction(...) (e.g.,
sendAction(NewLoanAccountAction.Internal.LoanCreated),
LoanCreationFailed(dataState), LoanCreationLoading) and remove direct
mutableStateFlow updates, delay, and sendEvent calls from the collector; then
implement handling for those internal actions in handleAction (handle
NewLoanAccountAction.Internal.LoanCreated by updating mutableStateFlow,
launching a coroutine to delay(1000) and then
sendEvent(NewLoanAccountEvent.LoanCreationSuccess(state.clientId)), and handle
error/loading variants similarly) so all state mutations and business flow live
in the reducer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fdbdc3a9-726b-45bd-85cf-ef74459643b8
📒 Files selected for processing (5)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
📜 Review details
⏰ 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 context used
📓 Path-based instructions (3)
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: Additional Code Review Guidelines:
- Null Safety & Stability
- Avoid using
!!operator- Handle null cases explicitly using safe calls or proper state handling
- Do not assume values are always non-null without guarantees
- Architecture Boundaries
- ViewModel must not depend on specific network/library implementations
- Ensure proper separation between data, domain, and presentation layers
- Do not format data (currency, dates, calculations) inside the UI layer
- All formatting must be handled in the ViewModel and exposed via state (e.g., StateFlow)
- Performance Considerations
- Avoid unnecessary recompositions in Compose
- Do not attach heavy logic to frequently changing states (e.g., scrollState)
- Prefer lifting state up instead of recomputing in child composables
- Compose & Navigation Best Practices
- NEVER trigger navigation functions or side-effects directly during composition
- Always wrap navigation calls inside
LaunchedEffectorEventsEffectto avoid repeated execution on recomposition- Avoid triggering intensive side-effects during recomposition
- Navigation routes must be type-safe.
- Ensure all route classes or objects used for navigation are annotated with
@Serializable.
- UI Structure
- Dialogs must be separated into their own composables
- Do not embed dialogs inline within complex main screens
- Localization Consistency
- Ensure all supported languages are updated consistently across modules
- Verify translations exist for newly added UI strings
- Code Cleanliness
- Avoid unnecessary inline comments unless critical
- Remove leftover debug or commented code
- Focus on correctness, readability, and maintainability over cosmetic nitpicks.
- Avoid reviewing README, config, or asset files.
- Prioritize identifying bugs, performance issues, and architectural concerns.
- Naming & Intent Rules:
- Follow the official Kotlin Coding Conventions:
https://kotlinlang.org/docs/coding-conventions.html- Use self-explanatory ...
Files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
**/*Screen.kt
⚙️ CodeRabbit configuration file
**/*Screen.kt: Screen architecture rules:Each screen must follow a 2-layer structure:
Layer 1 (Entry/Route Composable):
- Function Name: Typically
*ScreenRouteor the entry-point composable.- Responsibilities: Inject ViewModel (e.g.,
koinViewModel), take navigation lambdas, collect theStateFlow, and handle ViewModel events.- Logic: Should only handle ViewModel interaction, state collection, and triggering navigation in response to ViewModel events.
Layer 2 (Stateful/Content Composable —
*Screenor*ScreenContent):
- Parameters: MUST only take
state(the UI state object) and a singleonActionlambda (e.g.,onAction: (FeatureAction) -> Unit).- Responsibilities: Render the UI based strictly on the provided
state.- Rule: MUST NOT pass multiple separate lambda functions for different UI interactions; consolidate them into the single
onAction.- Constraint: Must NOT contain any business logic or ViewModel/Navigation references.
Internal/private helper composables (e.g., dialogs, sections, sub-components):
- These are NOT subject to the single
onActionrule.- They may accept specific, focused lambdas (e.g.,
onRetry: () -> Unit) or a narrowedonActionas appropriate.- However, they must NOT be passed the ViewModel or navigation controllers directly.
UI consistency:
- Avoid hardcoded values (dp, sp, padding, fontSize, colors)
- Use DesignToken, KptTheme, AppColors, MifosTypography for spacing, typography, and colors
Code quality:
- Keep Composables small and readable
- Avoid deeply nested UI
Flag:
- Missing Layer 2 (
*Screen/*ScreenContent) separation from the entry-point composable- Layer 2 (
*Screen/*ScreenContent) receiving multiple separate lambdas instead of a singleonAction- UI logic inside the entry-point composable
- Business logic inside any Composable
- Hardcoded strings instead of using string resources
- Hardcoded dp/sp values
- Direct styling instead of using DesignToken or KptTheme
Files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt
**/*ViewModel.kt
⚙️ CodeRabbit configuration file
**/*ViewModel.kt: MVI architecture rules:
- All new features must follow MVI
- ViewModel must extend BaseViewModel
- The ViewModel MUST maintain a single UI state (e.g., a single StateFlow)
instead of multiple separate state variables.Required:
- Use *State, *Event, *Action
- Naming must be consistent (FeatureViewModel, FeatureState, FeatureEvent, FeatureAction)
- Follow unidirectional flow:
Action → ViewModel → State → UIInternal reducer/action architecture rules:
Async operations MUST NOT directly mutate UI state repeatedly inside
Flow collectors, suspend callbacks, or repository result handlers.Repository/network/database results MUST be converted into internal
actions usingsendAction(...).
handleAction(...)must act as the primary reducer responsible for:
- state mutation
- reducer-style state transitions
- triggering follow-up actions
Large async methods must be split into:
- async collection layer
- internal action dispatching
- reducer/state handling
Avoid directly calling another business/data-loading method from
repository collectors or async callbacks.
Prefer dispatching follow-up internal actions instead.Preferred pattern:
repository result
-> sendAction(...)
-> handleAction(...)
-> mutableStateFlow.update { ... }Anti-pattern examples:
mutableStateFlow.update { ... } inside collect { }
fetchX() -> directly calls fetchY() inside async collector
large methods mixing:
- collection
- state mutation
- navigation
- business chainingFlag:
- Multiple
mutableStateFlow.update {}calls inside collect { }- Direct state mutation inside async repository callbacks
- Async methods performing both collection and reducer logic
- Direct business-flow chaining from async collectors
- Missing internal reducer actions for async results
- Missing *State / *Event / *Action
- ViewModel not...
Files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
🧠 Learnings (4)
📚 Learning: 2026-02-06T13:15:16.968Z
Learnt from: sahilshivekar
Repo: openMF/android-client PR: 2603
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt:43-106
Timestamp: 2026-02-06T13:15:16.968Z
Learning: Guideline: When a Kotlin function parameter is nullable (e.g., balance: Double?, currencyCode: String?, maximumFractionDigits: Int?) and downstream calls require a non-null value, add null-safety handling in all implementations. Specifically, avoid calling Currency.getInstance(currencyCode) with a possibly null currencyCode; provide a safe default (e.g., currencyCode ?: "$") or validate before use. Ensure all platform targets (Android/Desktop/Native) follow consistent null handling, and consider centralizing currencyCode normalization in the common layer if feasible. Add tests covering null currencyCode to prevent NPEs.
Applied to files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
📚 Learning: 2026-04-01T05:03:14.323Z
Learnt from: kartikey004
Repo: openMF/mifos-x-field-officer-app PR: 2659
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanDisbursement/LoanAccountDisbursementScreen.kt:190-195
Timestamp: 2026-04-01T05:03:14.323Z
Learning: In this repo, existing `SelectableDates` / `SelectableDates.isSelectableDate(utcTimeMillis: Long)` implementations use `Clock.System.now().toEpochMilliseconds()` (UTC epoch millis) for date-boundary checks. During PR reviews, do not flag these checks for not using a timezone-aware `LocalDate` start-of-day approach; treat it as an established project-wide pattern. If a change is desired, handle it as a coordinated project-wide improvement rather than as a per-PR review issue.
Applied to files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
📚 Learning: 2026-03-16T11:58:32.496Z
Learnt from: sahilshivekar
Repo: openMF/mifos-x-field-officer-app PR: 2647
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt:96-96
Timestamp: 2026-03-16T11:58:32.496Z
Learning: In feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt (Kotlin/Compose Multiplatform), navigateToDashboard uses a direct method reference navController::navigateToLoanDashboardScreen without an ID guard. This is intentional: LoanAccountProfileScreen always provides a valid loanId, and LoanDashboardScreen handles invalid/missing data by showing a 'Failed to fetch Loan details' error state. Do not flag the absence of an ID guard here in future reviews.
Applied to files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
📚 Learning: 2026-02-16T08:37:28.351Z
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersList/ClientIdentifiersListScreen.kt:60-60
Timestamp: 2026-02-16T08:37:28.351Z
Learning: In Kotlin files under the android-client module, replace MaterialTheme references with the established KptTheme import: import template.core.base.designsystem.theme.KptTheme. This should be applied consistently across files where KptTheme is used, replacing any MaterialTheme imports with the correct KptTheme import path.
Applied to files:
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt
🔇 Additional comments (8)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (1)
1128-1128: LGTM!The
LoanCreationSuccessevent clearly separates successful loan creation from generic finish/cancel flows, following the MVI event naming convention.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (3)
91-91: LGTM!The
onLoanCreatedcallback follows proper naming conventions and is correctly placed in the entry-point composable (Layer 1).
101-101: LGTM!Navigation is properly triggered inside
EventsEffect, avoiding recomposition issues. This follows the coding guidelines requirement to wrap navigation calls inLaunchedEffectorEventsEffect.
242-245: LGTM!The dialog now focuses solely on showing the snackbar message, correctly delegating the navigation timing to the ViewModel. This separates UI feedback from business flow control.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountRoute.kt (1)
23-37: LGTM!The
onLoanCreatedcallback is properly threaded through the destination builder and forwarded to the screen composable, maintaining consistency with the existing navigation pattern.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt (2)
44-44: LGTM!The
onLoanCreatedcallback parameter follows the established pattern for navigation callbacks inloanDestination.
77-82: LGTM!The three-callback pattern correctly separates different exit flows:
onNavigateBack: explicit cancel/back actiononFinish: completing the stepper without submissiononLoanCreated: successful loan creation → navigation to loan listfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
381-386: LGTM!The navigation correctly:
- Navigates to the client's loan accounts screen with the newly created
clientId- Removes
NewLoanAccountRoutefrom the back stack (popUpTo<NewLoanAccountRoute> { inclusive = true }) to prevent users from navigating back into the completed creation flow- Uses
launchSingleTop = trueto prevent duplicate destinations whenClientLoanAccountsRouteis already at the top of the stack afterpopUpTo
|
@gurnoorpannu Resolve Merge Conflicts |
|
@gurnoorpannu Please fix the static checks. |
|
|
3 participants
Fixes - MIFOSAC-776
Before
Loan.Creation.webm
After
Screen_recording_20260505_223326.webm