feat(loan): implement loan account screen#2626
Merged
therajanmaurya merged 4 commits intoopenMF:developmentfrom Mar 2, 2026
Merged
feat(loan): implement loan account screen#2626therajanmaurya merged 4 commits intoopenMF:developmentfrom
therajanmaurya merged 4 commits intoopenMF:developmentfrom
Conversation
kartikey004
force-pushed
the
feature/loan-account-profile
branch
from
b9a2ceb to
406846e
Compare
|
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:
📝 WalkthroughWalkthroughAdds a Loan Account Profile feature: new navigation/routes, ViewModel, composable screen and components, status enums/UI models, design-system colors, strings, drawable icons, DI registration, and build/dependency updates. Changes
Sequence DiagramsequenceDiagram
actor User
participant Nav as Navigation
participant Screen as LoanAccountProfileScreen
participant VM as LoanAccountProfileViewModel
participant Repo as LoanAccountSummaryRepository
participant API as BackendAPI
User->>Nav: navigateToLoanAccountProfileScreen(loanId)
Nav->>Screen: initialize composable with loanId
Screen->>VM: obtain ViewModel / provide loanId
VM->>Repo: request loan details(loanId)
Repo->>API: fetch LoanWithAssociations
API-->>Repo: return loan data
Repo-->>VM: emit DataState.Success(loan)
VM->>VM: compute LoanProfileStatus & LoanStatusUiModel (label, color)
VM-->>Screen: emit LoanAccountState with loan & status UI
Screen->>Screen: render top card, status badge, overview, action items
User->>Screen: tap action (Approve/Repayment/Transfer/Detail)
Screen->>VM: dispatch action (OnNextActionClick / OnDetailItemClick)
VM-->>Screen: emit LoanAccountEvent (NavigateToAction / NavigateToDetail)
Screen->>Nav: navigate to corresponding flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
biplab1
reviewed
Feb 27, 2026
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/Color.kt
Outdated
Show resolved
Hide resolved
biplab1
reviewed
Feb 27, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.kt`:
- Around line 23-35: The destination contract loanProfileAccountDestination is
missing a detail-navigation callback/path, so add a parameter (e.g.,
onDetailClick: (DetailItemType) -> Unit) to loanProfileAccountDestination and
pass it into LoanAccountProfileScreen; update the composable invocation in
loanProfileAccountDestination to include onDetailClick, and ensure
LoanAccountProfileScreen consumes its event LoanAccountEvent.NavigateToDetail by
invoking onDetailClick(event.detailItem). Adjust any callers of
loanProfileAccountDestination to provide the new callback.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt`:
- Around line 222-223: The title currently interpolates
loanAccount.loanProductName nullable directly, which can render "null"; update
the text construction in LoanAccountProfileScreen so loanProductName is replaced
with a safe fallback or omitted when absent (e.g., use
loanAccount.loanProductName?.uppercase() ?: "" or build the title with non-null
parts only and trim) and ensure accountNo is always included; modify the
expression used for text (currently the template with
loanAccount.loanProductName?.uppercase() and loanAccount.accountNo) to avoid
emitting the literal "null".
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt`:
- Around line 64-74: In the DataState.Success branch in
LoanAccountProfileViewModel (inside the mutableStateFlow.update block),
explicitly handle the case where result.data is null: do not clear dialogs or
compute status UI/nextAction resources from a null payload; instead update the
state to a recoverable-error state (e.g., set dialogState to an error/retry
dialog, keep the existing loanAccount unchanged or set a safe fallback, and
avoid calling calculateStatusUi/calculateNextActionResource). Locate the
DataState.Success branch and add a null-check on result.data so you only compute
currentStatus and call calculateStatusUi/calculateNextActionResource when
result.data is non-null; otherwise set an appropriate error dialog state and
preserve previous data to allow recovery.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/Color.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktfeature/loan/src/commonMain/composeResources/drawable/account_details.xmlfeature/loan/src/commonMain/composeResources/drawable/autorenew.xmlfeature/loan/src/commonMain/composeResources/drawable/charges.xmlfeature/loan/src/commonMain/composeResources/drawable/collateral.xmlfeature/loan/src/commonMain/composeResources/drawable/dashboard.xmlfeature/loan/src/commonMain/composeResources/drawable/design_services.xmlfeature/loan/src/commonMain/composeResources/drawable/documents.xmlfeature/loan/src/commonMain/composeResources/drawable/notes.xmlfeature/loan/src/commonMain/composeResources/drawable/originators.xmlfeature/loan/src/commonMain/composeResources/drawable/repayment_schedule.xmlfeature/loan/src/commonMain/composeResources/drawable/reschedules.xmlfeature/loan/src/commonMain/composeResources/drawable/term_variations.xmlfeature/loan/src/commonMain/composeResources/drawable/transaction.xmlfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/components/LoanAccountProfileActionItem.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
.../commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.kt
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt
Outdated
Show resolved
Hide resolved
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt
Show resolved
Hide resolved
Contributor
|
@kartikey004 It looks like this actions button (right arrow) was missed: 
|
Contributor
|
Also, please check the UI parameters like corner radius, it should match as used in the Client Flow. |
Member
therajanmaurya
left a comment
therajanmaurya
left a comment
There was a problem hiding this comment.
PR Review: Loan Account Profile Screen
Summary
This PR implements a new Loan Account Profile screen with a clean architecture following the existing Client Profile pattern. The implementation includes proper ViewModel setup, type-safe navigation, status-based UI, and comprehensive action items.
✅ Strengths
- Clean Architecture: Good separation with ViewModel, State, Events, and Actions following MVI pattern
- Type-Safe Navigation: Uses Kotlin serialization for route parameters (
LoanAccountRoute) - Status Handling: Well-organized
LoanProfileStatusenum with proper color mapping viaLoanStatusUiModel - Comprehensive UI: Full set of 13 action items with proper icons and descriptions
- Preview Support: Includes
PreviewParameterProviderfor multiple states (content, loading, error) - Consistent Patterns: Follows the same patterns established in Client Profile
⚠️ Items to Verify
1. EventsEffect Navigation Pattern
File: LoanAccountProfileScreen.kt:71-89
The EventsEffect block handles navigation events. Please verify that your EventsEffect utility properly consumes events (e.g., using Channel or SharedFlow with replay=0) to prevent multiple navigation calls on recomposition.
This is the same pattern we discussed in PR #2623 - just want to ensure consistency.
2. Copyright Year Inconsistency
Some new files use Copyright 2026 while others use Copyright 2025:
LoanAccountProfileNavigation.kt→ 2026LoanAccountProfileActionItem.kt→ 2026- Other files → 2025
Consider aligning to 2025 for consistency with the rest of the codebase.
🔍 Code Quality
| Aspect | Rating |
|---|---|
| Architecture | ✅ Excellent |
| Navigation | ✅ Good |
| State Management | ✅ Good |
| UI Consistency | ✅ Good |
| Error Handling | ✅ Good |
| Accessibility |
💡 Suggestions for Future
- Loading State UX: Consider showing card skeleton while loading
- Error Recovery: Add "Go Back" option alongside retry on error states
- Status Badge: Add an icon alongside color for accessibility
Verdict
The implementation is well-structured and follows established patterns. The dynamic action button based on loan status (Approve/Repayment/Transfer) is a nice UX touch.
Good work! 👍
Note: The Transfer Funds action for Overpaid status is marked as TODO - this is acknowledged in the PR description as a separate ticket (MIFOSAC-658).
| val state by viewModel.stateFlow.collectAsStateWithLifecycle() | ||
|
|
||
| EventsEffect(viewModel.eventFlow) { event -> | ||
| when (event) { |
Member
There was a problem hiding this comment.
Verify EventsEffect Implementation
Please confirm that EventsEffect internally uses LaunchedEffect with proper event consumption to prevent navigation from firing multiple times.
If it follows this pattern, it's correct:
@Composable
fun <T> EventsEffect(events: Flow<T>, handler: suspend (T) -> Unit) {
LaunchedEffect(Unit) {
events.collect { handler(it) }
}
}If it doesn't consume events properly, wrap navigation in LaunchedEffect(event) to ensure one-time execution.
.../commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.kt
Outdated
Show resolved
Hide resolved
Contributor
|
@kartikey004 Is it better to just show at most two screenshots side-by-side in the PR description for better visibility. |
Contributor
Author
@biplab1 Oh yes, I missed that. can you please specify which screen this button should navigate to? |
Contributor
|
@kartikey004 It should navigate to Loan Account Actions Screen, not to be implemented here. It is a separate ticket. |
Contributor
Please make the width wider. It is still showing smaller images. Also, I didn't tell you to remove the 3rd image, just that keep only two in one row. |
Member
therajanmaurya
left a comment
therajanmaurya
left a comment
There was a problem hiding this comment.
Good implementation of the Loan Account Profile screen! The architecture is clean and follows the established Client Profile pattern.
What I noticed:
- Clean MVI pattern with proper ViewModel, State, Events, and Actions
- Type-safe navigation using Kotlin serialization
- Good status handling with color-coded UI based on loan state
- Comprehensive action items list for loan management
Items discussed in previous comments:
EventsEffectpattern - please verify it handles event consumption correctly- Copyright year inconsistency (2026 vs 2025) in some files
- Color naming convention (prefix with
loan) - XML icons should use Material Icons
Suggestions:
- Consider showing a skeleton loader while loading instead of just a progress indicator
- The status badge uses color alone - consider adding an icon for accessibility
Overall solid work! The navigation wiring and status-based dynamic action button is a nice touch.
biplab1
reviewed
Feb 27, 2026
feature/loan/src/commonMain/composeResources/values/strings.xml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/Color.kt (1)
252-260: Consider consolidating similar loan color constants.There are now two sets of loan-related colors:
loanIndicator*(lines 252-255) andloanStatus*(lines 257-260). Both appear to represent similar concepts (Active, Pending states). If they serve the same semantic purpose, consider consolidating to avoid duplication and inconsistency.If they intentionally serve different UI contexts (e.g., indicators vs. badges), a brief comment documenting the distinction would help maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/Color.kt` around lines 252 - 260, The file defines two overlapping sets of loan colors: loanIndicatorActive/loanIndicatorPending/loanIndicatorWaitingForDisbursal/loanIndicatorOther and loanActiveStatus/loanPendingStatus/loanOverpaidStatus/loanUnknownStatus; either consolidate them into one canonical set (e.g., replace usages of loanIndicator* with loanActiveStatus/loanPendingStatus etc. or vice versa) to remove duplication and ensure consistent values, or if they are intentionally different, add a brief KDoc comment above each group (referencing loanIndicatorActive and loanActiveStatus) explaining their distinct UI contexts (indicator vs badge) and keep their names/values consistent and clearly documented.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt (1)
78-85: Avoid silent no-op flows for Transfer and Account Details actions.Line 99 and Line 104 currently swallow user actions with no feedback/navigation. Since these actions are visibly tappable, this creates a dead-end UX. Please thread explicit callbacks (or temporarily disable those affordances until implemented).
🔧 Suggested wiring to prevent dead-end interactions
internal fun LoanAccountProfileScreen( onNavigateBack: () -> Unit, approveLoan: (Int, LoanWithAssociationsEntity) -> Unit, onRepaymentClick: (LoanWithAssociationsEntity) -> Unit, + onTransferClick: (LoanWithAssociationsEntity) -> Unit, + onAccountDetailsClick: (LoanWithAssociationsEntity) -> Unit, onDetailItemClick: (LoanAccountProfileActionItem) -> Unit, navController: NavController, @@ when (event.action) { LoanProfileAction.Approve -> approveLoan(account.id, account) LoanProfileAction.Repayment -> onRepaymentClick(account) - LoanProfileAction.Transfer -> { - // TODO: Ticket in progress (MIFOSAC-658) - } + LoanProfileAction.Transfer -> onTransferClick(account) } } is LoanAccountEvent.NavigateToDetail -> onDetailItemClick(event.detailItem) - LoanAccountEvent.NavigateToAccountDetails -> {} + LoanAccountEvent.NavigateToAccountDetails -> onAccountDetailsClick(account) } }Also applies to: 95-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt` around lines 78 - 85, LoanAccountProfileScreen currently swallows taps for Transfer and Account Details, creating dead-end UX; update the onDetailItemClick handling in LoanAccountProfileScreen to forward those actions via the existing onDetailItemClick callback (use LoanAccountProfileActionItem.Transfer and .AccountDetails) or, if not implemented, disable the tappable affordances for those items so they are not interactable. Locate the touch handlers/rendering logic for the action list inside LoanAccountProfileScreen and either invoke onDetailItemClick(action) for the Transfer and AccountDetails cases or set their enabled/visible state to false (and update any accessibility labels) to avoid silent no-op behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt`:
- Around line 119-137: handleAction currently swallows
LoanAccountAction.OnAccountClick and handleNextAction drops the
LoanProfileStatus.UNKNOWN path; instead dispatch explicit events so UI feedback
isn't lost: in handleAction (function handleAction) replace the empty
OnAccountClick branch with a sendEvent call (e.g.,
sendEvent(LoanAccountEvent.NavigateToAccount) or a new
LoanAccountEvent.AccountClicked) and in handleNextAction handle the
LoanProfileStatus.UNKNOWN branch by emitting a fallback event (e.g.,
sendEvent(LoanAccountEvent.ShowUnsupportedAction) or ShowToast) so the app shows
feedback; add the corresponding LoanAccountEvent variants if they don't exist.
---
Nitpick comments:
In
`@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/Color.kt`:
- Around line 252-260: The file defines two overlapping sets of loan colors:
loanIndicatorActive/loanIndicatorPending/loanIndicatorWaitingForDisbursal/loanIndicatorOther
and loanActiveStatus/loanPendingStatus/loanOverpaidStatus/loanUnknownStatus;
either consolidate them into one canonical set (e.g., replace usages of
loanIndicator* with loanActiveStatus/loanPendingStatus etc. or vice versa) to
remove duplication and ensure consistent values, or if they are intentionally
different, add a brief KDoc comment above each group (referencing
loanIndicatorActive and loanActiveStatus) explaining their distinct UI contexts
(indicator vs badge) and keep their names/values consistent and clearly
documented.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt`:
- Around line 78-85: LoanAccountProfileScreen currently swallows taps for
Transfer and Account Details, creating dead-end UX; update the onDetailItemClick
handling in LoanAccountProfileScreen to forward those actions via the existing
onDetailItemClick callback (use LoanAccountProfileActionItem.Transfer and
.AccountDetails) or, if not implemented, disable the tappable affordances for
those items so they are not interactable. Locate the touch handlers/rendering
logic for the action list inside LoanAccountProfileScreen and either invoke
onDetailItemClick(action) for the Transfer and AccountDetails cases or set their
enabled/visible state to false (and update any accessibility labels) to avoid
silent no-op behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/Color.ktcore/ui/build.gradle.ktscore/ui/src/commonMain/composeResources/drawable/account_details.xmlcore/ui/src/commonMain/composeResources/drawable/autorenew.xmlcore/ui/src/commonMain/composeResources/drawable/charges.xmlcore/ui/src/commonMain/composeResources/drawable/collateral.xmlcore/ui/src/commonMain/composeResources/drawable/dashboard.xmlcore/ui/src/commonMain/composeResources/drawable/design_services.xmlcore/ui/src/commonMain/composeResources/drawable/documents.xmlcore/ui/src/commonMain/composeResources/drawable/notes.xmlcore/ui/src/commonMain/composeResources/drawable/originators.xmlcore/ui/src/commonMain/composeResources/drawable/repayment_schedule.xmlcore/ui/src/commonMain/composeResources/drawable/reschedules.xmlcore/ui/src/commonMain/composeResources/drawable/term_variations.xmlcore/ui/src/commonMain/composeResources/drawable/transaction.xmlfeature/loan/build.gradle.ktsfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/components/LoanAccountProfileActionItem.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
✅ Files skipped from review due to trivial changes (3)
- core/ui/src/commonMain/composeResources/drawable/documents.xml
- core/ui/src/commonMain/composeResources/drawable/notes.xml
- core/ui/src/commonMain/composeResources/drawable/design_services.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt (2)
61-99: Consider cancelling in-flight loads on retry.If
OnRetryis triggered while a previousloadLoanAccountDetailscall is still collecting, both coroutines will update state. While the repository likely emits quickly, tracking and cancelling the job would prevent potential race conditions during rapid retries.💡 Optional: Track and cancel previous job
+ private var loadJob: kotlinx.coroutines.Job? = null + private fun loadLoanAccountDetails(loanId: Int) { - viewModelScope.launch { + loadJob?.cancel() + loadJob = viewModelScope.launch { loanRepository.getLoanById(loanId).collect { result -> // ... existing logic } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt` around lines 61 - 99, The loadLoanAccountDetails coroutine can run concurrently on retries; add a cancellable Job field (e.g., private var loanDetailsJob: Job?) to the ViewModel, cancel any existing job (loanDetailsJob?.cancel()) before starting a new one, and assign loanDetailsJob = viewModelScope.launch { ... } so the previous collection is cancelled and only the latest call updates mutableStateFlow; keep all existing logic inside the launched block and clear/replace loanDetailsJob appropriately when complete or cancelled.
140-149: Consider adding a brief comment documenting the flag precedence.The ordering
pendingApproval → overpaid → activedetermines which status wins if multiple flags are true. A brief inline comment would clarify this intentional priority for future maintainers.📝 Example documentation
private val LoanStatusEntity?.toProfileStatus: LoanProfileStatus get() { if (this == null) return LoanProfileStatus.UNKNOWN + // Priority: pending approval > overpaid > active return when { this.pendingApproval == true -> LoanProfileStatus.PENDING this.overpaid == true -> LoanProfileStatus.OVERPAID this.active == true -> LoanProfileStatus.ACTIVE else -> LoanProfileStatus.UNKNOWN } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt` around lines 140 - 149, Add a brief inline comment above the LoanStatusEntity?.toProfileStatus extension getter explaining the intentional precedence of flags (pendingApproval -> overpaid -> active) so maintainers know which flag wins when multiple are true; reference the getter name LoanStatusEntity?.toProfileStatus and the enum LoanProfileStatus, and note that pendingApproval is checked first, then overpaid, then active, with UNKNOWN as fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt`:
- Around line 61-99: The loadLoanAccountDetails coroutine can run concurrently
on retries; add a cancellable Job field (e.g., private var loanDetailsJob: Job?)
to the ViewModel, cancel any existing job (loanDetailsJob?.cancel()) before
starting a new one, and assign loanDetailsJob = viewModelScope.launch { ... } so
the previous collection is cancelled and only the latest call updates
mutableStateFlow; keep all existing logic inside the launched block and
clear/replace loanDetailsJob appropriately when complete or cancelled.
- Around line 140-149: Add a brief inline comment above the
LoanStatusEntity?.toProfileStatus extension getter explaining the intentional
precedence of flags (pendingApproval -> overpaid -> active) so maintainers know
which flag wins when multiple are true; reference the getter name
LoanStatusEntity?.toProfileStatus and the enum LoanProfileStatus, and note that
pendingApproval is checked first, then overpaid, then active, with UNKNOWN as
fallback.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt
TheKalpeshPawar
suggested changes
Feb 28, 2026
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt
Show resolved
Hide resolved
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt
Show resolved
Hide resolved
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt
Outdated
Show resolved
Hide resolved
kartikey004
force-pushed
the
feature/loan-account-profile
branch
from
59dacc7 to
ed7b92c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/components/LoanAccountProfileActionItem.kt (1)
130-144: Consider making this list exhaustive via reflection orsealedSubclasses.If a new action item is added to the sealed class but forgotten in
loanProfileActionItems, it will silently be omitted from the UI. Consider adding a compile-time or runtime check.💡 Optional: Add a runtime assertion to catch missing items
internal val loanProfileActionItems: ImmutableList<LoanAccountProfileActionItem> = persistentListOf( LoanAccountProfileActionItem.General, LoanAccountProfileActionItem.Dashboard, LoanAccountProfileActionItem.AccountDetails, LoanAccountProfileActionItem.RepaymentSchedule, LoanAccountProfileActionItem.Transactions, LoanAccountProfileActionItem.Charges, LoanAccountProfileActionItem.Originators, LoanAccountProfileActionItem.Collateral, LoanAccountProfileActionItem.TermVariations, LoanAccountProfileActionItem.Reschedules, LoanAccountProfileActionItem.Documents, LoanAccountProfileActionItem.Notes, LoanAccountProfileActionItem.StandingInstructions, ).also { list -> check(list.size == LoanAccountProfileActionItem::class.sealedSubclasses.size) { "loanProfileActionItems is missing some LoanAccountProfileActionItem subclasses" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/components/LoanAccountProfileActionItem.kt` around lines 130 - 144, The current persistentListOf assigned to loanProfileActionItems can drift from the sealed class; replace the manual list with one derived from LoanAccountProfileActionItem::class.sealedSubclasses (mapping each subclass to its objectInstance or instantiating if needed) so all subclasses are included, and add a runtime assertion (e.g., check(...)) that the resulting list size equals sealedSubclasses.size to fail fast if any subclass is missing; ensure you keep the symbol names loanProfileActionItems and LoanAccountProfileActionItem so callers remain unchanged.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt (1)
98-105:TransferandNavigateToAccountDetailsevents are no-ops.Line 98-100:
Transferaction is a TODO (acceptable per PR description).
Line 104:NavigateToAccountDetailsis empty. If this is intentional, consider adding a comment; otherwise, implement the navigation.📝 Add clarifying comments for placeholder handlers
LoanProfileAction.Transfer -> { - // TODO: Ticket in progress (MIFOSAC-658) + // TODO: Transfer Funds screen - Ticket in progress (MIFOSAC-658) } } } is LoanAccountEvent.NavigateToDetail -> onDetailItemClick(event.detailItem) - LoanAccountEvent.NavigateToAccountDetails -> {} + LoanAccountEvent.NavigateToAccountDetails -> { + // TODO: Navigate to account details screen when implemented + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt` around lines 98 - 105, The empty handlers for LoanProfileAction.Transfer and LoanAccountEvent.NavigateToAccountDetails are ambiguous; update LoanAccountProfileScreen to clarify intent by either (A) adding explicit comments in the Transfer case and the NavigateToAccountDetails branch indicating "TODO: implement navigation (MIFOSAC-658) / intentionally no-op" or (B) implementing the navigation by calling the appropriate navigation handler (e.g., invoke the existing onDetailItemClick or the component's account-details navigation callback) so the NavigateToAccountDetails branch is not empty; reference the LoanProfileAction.Transfer case and the LoanAccountEvent.NavigateToAccountDetails branch to locate and adjust the code.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt (2)
140-149: Status priority may produce unexpected results when multiple flags are true.If
pendingApproval == trueandoverpaid == truesimultaneously (edge case from backend), the current order returnsPENDING. Verify this is the intended business logic.💬 Consider documenting the priority or adding defensive logic
private val LoanStatusEntity?.toProfileStatus: LoanProfileStatus get() { if (this == null) return LoanProfileStatus.UNKNOWN + // Priority: PENDING > OVERPAID > ACTIVE > UNKNOWN return when { this.pendingApproval == true -> LoanProfileStatus.PENDING this.overpaid == true -> LoanProfileStatus.OVERPAID this.active == true -> LoanProfileStatus.ACTIVE else -> LoanProfileStatus.UNKNOWN } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt` around lines 140 - 149, The extension property LoanStatusEntity?.toProfileStatus currently returns PENDING if multiple flags (pendingApproval, overpaid, active) are true which can be ambiguous; confirm the correct business priority and then make the code explicit: update LoanStatusEntity?.toProfileStatus to implement the agreed deterministic priority (e.g., check overpaid before pendingApproval if overpaid should win, or vice‑versa) by reordering or adding explicit combined-condition checks, add a brief comment documenting the chosen priority next to the extension, and include a unit test and/or a defensive log/warning when more than one flag is true to surface backend edge cases.
61-98: Consider checking network status before making a network request on retry.When the user taps retry after a network error,
loadLoanAccountDetailsis called without verifying network connectivity first. This could result in an immediate failure if still offline.🔌 Proposed improvement
private fun loadLoanAccountDetails(loanId: Int) { + if (!mutableStateFlow.value.networkConnection) { + mutableStateFlow.update { + it.copy(dialogState = LoanAccountState.DialogState.Error(Res.string.feature_loan_profile_error_details_not_found)) + } + return + } viewModelScope.launch { loanRepository.getLoanById(loanId).collect { result ->Alternatively, consider showing a more specific "No network connection" error message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt` around lines 61 - 98, The loadLoanAccountDetails function currently always calls loanRepository.getLoanById even when offline; inject or use an existing connectivity checker (e.g., ConnectivityObserver, NetworkMonitor or an isNetworkAvailable() helper) in the ViewModel and, at the start of loadLoanAccountDetails, check network availability and if offline call mutableStateFlow.update to set a specific "No network connection" DialogState (instead of making the repository call) and return; if online proceed with viewModelScope.launch and loanRepository.getLoanById as before, and optionally enhance the DataState.Error branch to translate network-related errors into the same no-network DialogState for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt`:
- Around line 207-212: The Android and Desktop implementations of
CurrencyFormatter.format call Currency.getInstance(currencyCode) without
handling a null currencyCode; update both implementations
(CurrencyFormatter.android.kt and CurrencyFormatter.desktop.kt) to guard against
null by using a safe default before calling Currency.getInstance (e.g., val
safeCode = currencyCode ?: "$"), and ensure decimalPlaces is also handled safely
as the Native implementation does so the function returns a formatted string
even when loanAccount.currency?.code is null.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/components/LoanAccountProfileActionItem.kt`:
- Around line 130-144: The current persistentListOf assigned to
loanProfileActionItems can drift from the sealed class; replace the manual list
with one derived from LoanAccountProfileActionItem::class.sealedSubclasses
(mapping each subclass to its objectInstance or instantiating if needed) so all
subclasses are included, and add a runtime assertion (e.g., check(...)) that the
resulting list size equals sealedSubclasses.size to fail fast if any subclass is
missing; ensure you keep the symbol names loanProfileActionItems and
LoanAccountProfileActionItem so callers remain unchanged.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt`:
- Around line 98-105: The empty handlers for LoanProfileAction.Transfer and
LoanAccountEvent.NavigateToAccountDetails are ambiguous; update
LoanAccountProfileScreen to clarify intent by either (A) adding explicit
comments in the Transfer case and the NavigateToAccountDetails branch indicating
"TODO: implement navigation (MIFOSAC-658) / intentionally no-op" or (B)
implementing the navigation by calling the appropriate navigation handler (e.g.,
invoke the existing onDetailItemClick or the component's account-details
navigation callback) so the NavigateToAccountDetails branch is not empty;
reference the LoanProfileAction.Transfer case and the
LoanAccountEvent.NavigateToAccountDetails branch to locate and adjust the code.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt`:
- Around line 140-149: The extension property LoanStatusEntity?.toProfileStatus
currently returns PENDING if multiple flags (pendingApproval, overpaid, active)
are true which can be ambiguous; confirm the correct business priority and then
make the code explicit: update LoanStatusEntity?.toProfileStatus to implement
the agreed deterministic priority (e.g., check overpaid before pendingApproval
if overpaid should win, or vice‑versa) by reordering or adding explicit
combined-condition checks, add a brief comment documenting the chosen priority
next to the extension, and include a unit test and/or a defensive log/warning
when more than one flag is true to surface backend edge cases.
- Around line 61-98: The loadLoanAccountDetails function currently always calls
loanRepository.getLoanById even when offline; inject or use an existing
connectivity checker (e.g., ConnectivityObserver, NetworkMonitor or an
isNetworkAvailable() helper) in the ViewModel and, at the start of
loadLoanAccountDetails, check network availability and if offline call
mutableStateFlow.update to set a specific "No network connection" DialogState
(instead of making the repository call) and return; if online proceed with
viewModelScope.launch and loanRepository.getLoanById as before, and optionally
enhance the DataState.Error branch to translate network-related errors into the
same no-network DialogState for consistency.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/Color.ktcore/ui/build.gradle.ktscore/ui/src/commonMain/composeResources/drawable/account_details.xmlcore/ui/src/commonMain/composeResources/drawable/autorenew.xmlcore/ui/src/commonMain/composeResources/drawable/charges.xmlcore/ui/src/commonMain/composeResources/drawable/collateral.xmlcore/ui/src/commonMain/composeResources/drawable/dashboard.xmlcore/ui/src/commonMain/composeResources/drawable/design_services.xmlcore/ui/src/commonMain/composeResources/drawable/documents.xmlcore/ui/src/commonMain/composeResources/drawable/notes.xmlcore/ui/src/commonMain/composeResources/drawable/originators.xmlcore/ui/src/commonMain/composeResources/drawable/repayment_schedule.xmlcore/ui/src/commonMain/composeResources/drawable/reschedules.xmlcore/ui/src/commonMain/composeResources/drawable/term_variations.xmlcore/ui/src/commonMain/composeResources/drawable/transaction.xmlfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktfeature/loan/build.gradle.ktsfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/components/LoanAccountProfileActionItem.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
🚧 Files skipped from review as they are similar to previous changes (11)
- core/ui/src/commonMain/composeResources/drawable/term_variations.xml
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.kt
- core/ui/src/commonMain/composeResources/drawable/repayment_schedule.xml
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
- core/ui/src/commonMain/composeResources/drawable/account_details.xml
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt
- core/ui/src/commonMain/composeResources/drawable/transaction.xml
- core/ui/src/commonMain/composeResources/drawable/charges.xml
- core/ui/src/commonMain/composeResources/drawable/documents.xml
- feature/loan/build.gradle.kts
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt`:
- Around line 60-63: The error handling in LoanAccountProfileViewModel currently
reuses Res.string.feature_loan_profile_error_details_not_found for multiple
failure modes; update the branches that set mutableStateFlow to
LoanAccountState.DialogState.Error (where loanAccount == null and the other
similar branches) to use distinct error resources for the different failure
causes (e.g., feature_loan_profile_error_offline,
feature_loan_profile_error_not_found, feature_loan_profile_error_fetch_failed)
and ensure the code paths that detect offline/network failures vs. missing data
map to the appropriate new Res.string keys so users see a clear, actionable
message instead of the generic “details not found.”
- Around line 69-71: loadLoanAccountDetails starts a new collector each time
it's called (via viewModelScope.launch {
loanRepository.getLoanById(loanId).collect { ... } }) which allows overlapping
collectors; fix this by tracking and cancelling any previous load job before
starting a new one (add a nullable Job property, e.g. private var loadLoanJob:
Job? in the ViewModel, call loadLoanJob?.cancel() then assign loadLoanJob =
viewModelScope.launch { loanRepository.getLoanById(loanId).collect { ... } } ),
or alternatively switch to a single shared Flow with .flatMapLatest or use
launchIn with a single coroutine scope to ensure only one active collector for
loadLoanAccountDetails at a time.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt
Show resolved
Hide resolved
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt
Show resolved
Hide resolved
kartikey004
force-pushed
the
feature/loan-account-profile
branch
from
8db8e11 to
cbfa757
Compare
|
biplab1
approved these changes
Mar 2, 2026
Contributor
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
Looks good to me. This can be merged.
Contributor
|
@kartikey004 Please resolve the comments so it can be merged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes - Jira-#663
Description - Implemented the new Loan Account screen, keeping the UI/UX consistent with the existing Client Profile flow. It includes a dynamic top card, an actions list and proper ViewModel + type-safe navigation setup.
Note - This PR handles the ACTIVE, PENDING and OVERPAID loan statuses. The Overpaid (Transfer Funds) next action screen is still in progress under a separate ticket.
Screenshots -
status_active_navigation.webm
Summary by CodeRabbit
New Features
Chores