feat(loan): add loan account general screen#2629
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:
WalkthroughAdds a new Loan Account General feature: repository interface and implementation with DI binding, ViewModel that loads and formats loan data, UI composables and strings for the general screen, and navigation wiring from profile to the new screen. Changes
Sequence DiagramsequenceDiagram
participant UI as LoanAccountGeneralScreen
participant VM as LoanAccountGeneralViewModel
participant Repo as LoanAccountGeneralRepository
participant DM as DataManagerLoan
participant Net as NetworkMonitor
UI->>VM: collect state / dispatch actions
Net->>VM: network status updates
VM->>VM: init with loanId from route
VM->>Repo: getLoanById(loanId)
Repo->>DM: getLoanById(loanId)
DM-->>Repo: loan data / null
Repo-->>VM: emit DataState(loan)
VM->>VM: fillGeneralState(loan) — format dates/currency, calculate totals
VM-->>UI: updated LoanAccountGeneralState
UI->>UI: render cards, summary table, details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt (1)
103-103: Avoid propagating sentinel loan IDs to navigation.Line 103 falls back to
-1whenstate.loanAccountis null, which can trigger invalid-detail navigation and downstream fetches with an invalid id.♻️ Suggested guard-based fix
- is LoanAccountEvent.NavigateToDetail -> onDetailItemClick(state.loanAccount?.id ?: -1, event.detailItem) + is LoanAccountEvent.NavigateToDetail -> { + val loanId = state.loanAccount?.id ?: return@EventsEffect + onDetailItemClick(loanId, event.detailItem) + }🤖 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` at line 103, Avoid passing the sentinel -1 to navigation: update the LoanAccountEvent.NavigateToDetail handling to guard against null loanAccount instead of falling back to -1—check state.loanAccount (or its id) and only call onDetailItemClick with the real id when present; if loanAccount is null, no-op or handle error/edge-case explicitly so NavigateToDetail never receives an invalid id from LoanAccountEvent.NavigateToDetail or state.loanAccount?.id.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralNavigation.kt (1)
17-20: Consider documenting the sentinel default value.The default
loanId = -1acts as a sentinel for "no loan selected." While the ViewModel extracts this viaSavedStateHandle.toRoute<>(), consider adding a KDoc comment clarifying this convention. Alternatively, you could make the parameter non-defaulted to enforce callers always provide a valid ID.🤖 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/loanAccountGeneral/LoanAccountGeneralNavigation.kt` around lines 17 - 20, The data class LoanAccountGeneralRoute currently uses a sentinel default loanId = -1 to mean "no loan selected"; update the declaration by either removing the default so callers must supply an explicit ID, or add a KDoc above LoanAccountGeneralRoute explaining that loanId == -1 is the sentinel for no selection and how callers/ SavedStateHandle.toRoute<>() should handle it; reference the LoanAccountGeneralRoute data class and its loanId property when making the change so callers and the ViewModel behavior remain consistent.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralViewModel.kt (1)
79-85: Consider logging exceptions for debugging.The generic exception catch silently converts all exceptions to error messages. Consider logging the exception for debugging purposes while still showing a user-friendly message.
💡 Add logging for debugging
try { fillGeneralState(loan) } catch (e: Exception) { + // Consider using a logging framework + e.printStackTrace() mutableStateFlow.update { it.copy(dialogState = LoanAccountGeneralState.DialogState.Error(e.message.toString())) } }🤖 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/loanAccountGeneral/LoanAccountGeneralViewModel.kt` around lines 79 - 85, The catch block around fillGeneralState in LoanAccountGeneralViewModel currently swallows exceptions and only sets LoanAccountGeneralState.DialogState.Error with e.message; update this block to log the full exception (stack trace and message) before updating mutableStateFlow so developers can debug; use the existing logging mechanism available in the module (e.g., a logger instance or platform logger) and include contextual info like the function name (fillGeneralState / LoanAccountGeneralViewModel) when calling the logger.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralScreen.kt (3)
93-153: Consider showing content during loading instead of blank screen.When
dialogStateis not null (Loading or Error), the entire content is hidden (line 100). For a better UX, consider showing the content with a loading overlay or skeleton, especially if partial data might already be available. Currently, users see a blank screen until loading completes.💡 Alternative approach: show content with overlay
`@Composable` internal fun LoanAccountGeneralScreen( state: LoanAccountGeneralState, onAction: (LoanAccountGeneralAction) -> Unit, navController: NavController, modifier: Modifier = Modifier, ) { - if (state.dialogState == null) { - Column(modifier = modifier.fillMaxSize()) { + Box(modifier = modifier.fillMaxSize()) { + Column(modifier = Modifier.fillMaxSize()) { MifosBreadcrumbNavBar(navController = navController) // ... rest of content } + + // Show loading/error overlay on top of content + LoanAccountGeneralDialog( + state = state, + onAction = onAction, + ) } }🤖 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/loanAccountGeneral/LoanAccountGeneralScreen.kt` around lines 93 - 153, The screen currently hides all content when state.dialogState != null in LoanAccountGeneralScreen, causing a blank UI during Loading/Error; instead always render the main Column (so PerformanceHistoryCard, LoanSummaryTable, LoanDetailsSection still display) and conditionally render an overlay or dialog when state.dialogState is non-null (e.g., show a LoadingOverlay/ProgressIndicator or an ErrorDialog on top), keeping the existing nav bar and content layout; update the if (state.dialogState == null) guard to render content unconditionally and add a composable that inspects state.dialogState to show the overlay/dialog while leaving underlying components visible.
319-320: Same fragility issue with totals and colors.The
totalsandtotColorslists also rely on parallel indexing. Apply the same data class pattern for consistency and safety.🤖 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/loanAccountGeneral/LoanAccountGeneralScreen.kt` around lines 319 - 320, The totals and totColors lists are fragile due to parallel indexing; replace them with a single list of a small data class (e.g., TotalEntry with value: BigDecimal/Number and color: Color) and populate it from state.totalOriginal, state.totalPaid, state.totalWaived, state.totalWrittenOff, state.totalOutstanding, state.totalOverDue paired with the corresponding colors (textColor or KptTheme.colorScheme.primary), then use that List<TotalEntry> in the UI rendering code instead of the separate totals and totColors lists to ensure safety and clarity.
263-264: Parallel lists for amounts and colors are fragile.The
amountsandamtColorslists rely on matching indices. If one list is modified without the other, it will cause visual bugs. Consider using a data class or pair to keep them together.💡 Safer approach with paired data
data class AmountCell(val value: String, val color: Color) val amountCells = listOf( AmountCell(row.original, textColor), AmountCell(row.paid, KptTheme.colorScheme.primary), AmountCell(row.waived, KptTheme.colorScheme.primary), AmountCell(row.writtenOff, KptTheme.colorScheme.primary), AmountCell(row.outstanding, textColor), AmountCell(row.overDue, textColor), )🤖 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/loanAccountGeneral/LoanAccountGeneralScreen.kt` around lines 263 - 264, The two parallel lists amounts and amtColors are fragile; replace them with a single paired structure (e.g., a data class like AmountCell with properties value and color) and build a list amountCells containing entries for row.original, row.paid, row.waived, row.writtenOff, row.outstanding, row.overDue mapped to their respective colors; then update any iteration or indexing logic that used amounts[i] and amtColors[i] to use amountCells[i].value and amountCells[i].color (or destructure) so the value/color pairing cannot get out of sync.
🤖 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/loanAccountGeneral/LoanAccountGeneralViewModel.kt`:
- Around line 127-129: The mapping uses loan.approvedPrincipal for both
proposedAmount and approvedAmount in LoanAccountGeneralViewModel
(proposedAmount, approvedAmount), but LoanWithAssociationsEntity only exposes
principal and approvedPrincipal so you cannot read a proposedPrincipal; fix by
either adding a proposedPrincipal field to LoanWithAssociationsEntity (and
populate it from the source) and then use loan.proposedPrincipal for
proposedAmount, or explicitly decide which existing field should represent the
“proposed” value (principal or approvedPrincipal) and update the assignment
accordingly; update the ViewModel assignments (proposedAmount, approvedAmount)
and the entity definition (LoanWithAssociationsEntity) as a pair so the source
field exists and the mapping is correct.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralNavigation.kt`:
- Around line 17-20: The data class LoanAccountGeneralRoute currently uses a
sentinel default loanId = -1 to mean "no loan selected"; update the declaration
by either removing the default so callers must supply an explicit ID, or add a
KDoc above LoanAccountGeneralRoute explaining that loanId == -1 is the sentinel
for no selection and how callers/ SavedStateHandle.toRoute<>() should handle it;
reference the LoanAccountGeneralRoute data class and its loanId property when
making the change so callers and the ViewModel behavior remain consistent.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralScreen.kt`:
- Around line 93-153: The screen currently hides all content when
state.dialogState != null in LoanAccountGeneralScreen, causing a blank UI during
Loading/Error; instead always render the main Column (so PerformanceHistoryCard,
LoanSummaryTable, LoanDetailsSection still display) and conditionally render an
overlay or dialog when state.dialogState is non-null (e.g., show a
LoadingOverlay/ProgressIndicator or an ErrorDialog on top), keeping the existing
nav bar and content layout; update the if (state.dialogState == null) guard to
render content unconditionally and add a composable that inspects
state.dialogState to show the overlay/dialog while leaving underlying components
visible.
- Around line 319-320: The totals and totColors lists are fragile due to
parallel indexing; replace them with a single list of a small data class (e.g.,
TotalEntry with value: BigDecimal/Number and color: Color) and populate it from
state.totalOriginal, state.totalPaid, state.totalWaived, state.totalWrittenOff,
state.totalOutstanding, state.totalOverDue paired with the corresponding colors
(textColor or KptTheme.colorScheme.primary), then use that List<TotalEntry> in
the UI rendering code instead of the separate totals and totColors lists to
ensure safety and clarity.
- Around line 263-264: The two parallel lists amounts and amtColors are fragile;
replace them with a single paired structure (e.g., a data class like AmountCell
with properties value and color) and build a list amountCells containing entries
for row.original, row.paid, row.waived, row.writtenOff, row.outstanding,
row.overDue mapped to their respective colors; then update any iteration or
indexing logic that used amounts[i] and amtColors[i] to use amountCells[i].value
and amountCells[i].color (or destructure) so the value/color pairing cannot get
out of sync.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralViewModel.kt`:
- Around line 79-85: The catch block around fillGeneralState in
LoanAccountGeneralViewModel currently swallows exceptions and only sets
LoanAccountGeneralState.DialogState.Error with e.message; update this block to
log the full exception (stack trace and message) before updating
mutableStateFlow so developers can debug; use the existing logging mechanism
available in the module (e.g., a logger instance or platform logger) and include
contextual info like the function name (fillGeneralState /
LoanAccountGeneralViewModel) when calling the logger.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt`:
- Line 103: Avoid passing the sentinel -1 to navigation: update the
LoanAccountEvent.NavigateToDetail handling to guard against null loanAccount
instead of falling back to -1—check state.loanAccount (or its id) and only call
onDetailItemClick with the real id when present; if loanAccount is null, no-op
or handle error/edge-case explicitly so NavigateToDetail never receives an
invalid id from LoanAccountEvent.NavigateToDetail or state.loanAccount?.id.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanAccountGeneralRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanAccountGeneralRepositoryImp.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/Color.ktfeature/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/loanAccountGeneral/LoanAccountGeneralNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralViewModel.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/navigation/LoanNavigation.kt
There was a problem hiding this comment.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralScreen.kt (1)
100-146: Extract shared content into a single composable to prevent preview/runtime drift.The section layout is duplicated in both the runtime screen and preview, and it has already diverged (preview titles are hardcoded). Consider a shared
LoanAccountGeneralContent(...)used by both call sites.♻️ Proposed refactor
+@Composable +private fun LoanAccountGeneralContent( + state: LoanAccountGeneralState, + modifier: Modifier = Modifier, +) { + Column( + modifier = modifier + .fillMaxSize() + .verticalScroll(rememberScrollState()), + ) { + Text( + text = stringResource(Res.string.feature_loan_general_section_performance_history), + style = KptTheme.typography.labelLarge, + modifier = Modifier.padding(horizontal = KptTheme.spacing.md), + ) + Spacer(Modifier.height(DesignToken.spacing.medium)) + PerformanceHistoryCard( + state = state, + modifier = Modifier.padding(horizontal = KptTheme.spacing.md), + ) + Spacer(Modifier.height(DesignToken.spacing.largeIncreased)) + Text( + text = stringResource(Res.string.feature_loan_general_section_loan_summary), + style = KptTheme.typography.labelLarge, + modifier = Modifier.padding(horizontal = KptTheme.spacing.md), + ) + Spacer(Modifier.height(DesignToken.spacing.medium)) + LoanSummaryTable(state = state) + Spacer(Modifier.height(DesignToken.spacing.largeIncreased)) + Text( + text = stringResource(Res.string.feature_loan_general_section_loan_details), + style = KptTheme.typography.labelLarge, + modifier = Modifier.padding(horizontal = KptTheme.spacing.md), + ) + Spacer(Modifier.height(DesignToken.spacing.medium)) + LoanDetailsSection( + state = state, + modifier = Modifier.padding(horizontal = KptTheme.spacing.md), + ) + Spacer(Modifier.height(KptTheme.spacing.xl)) + } +}Also applies to: 442-471
🤖 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/loanAccountGeneral/LoanAccountGeneralScreen.kt` around lines 100 - 146, The column block in LoanAccountGeneralScreen (containing Text headers, PerformanceHistoryCard, LoanSummaryTable, LoanDetailsSection and Spacers) is duplicated between runtime and preview causing drift; extract that repeated UI into a single composable (e.g., LoanAccountGeneralContent(state: LoanAccountGeneralState, modifier: Modifier = Modifier)) that renders the Text headers, PerformanceHistoryCard, LoanSummaryTable and LoanDetailsSection with the same modifiers and spacing, then replace the inline column in LoanAccountGeneralScreen and the preview call sites with this new LoanAccountGeneralContent to ensure both share identical layout and strings.
🤖 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/loanAccountGeneral/LoanAccountGeneralScreen.kt`:
- Around line 100-146: The column block in LoanAccountGeneralScreen (containing
Text headers, PerformanceHistoryCard, LoanSummaryTable, LoanDetailsSection and
Spacers) is duplicated between runtime and preview causing drift; extract that
repeated UI into a single composable (e.g., LoanAccountGeneralContent(state:
LoanAccountGeneralState, modifier: Modifier = Modifier)) that renders the Text
headers, PerformanceHistoryCard, LoanSummaryTable and LoanDetailsSection with
the same modifiers and spacing, then replace the inline column in
LoanAccountGeneralScreen and the preview call sites with this new
LoanAccountGeneralContent to ensure both share identical layout and strings.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralScreen.kt
kartikey004
left a comment
kartikey004
left a comment
There was a problem hiding this comment.
@amanna13 could you please confirm if the font sizes used here are as intended? right now the headings feel slightly larger compared to other screens.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/composeResources/values/strings.xml`:
- Line 239: Update the string resource named
"feature_loan_general_summary_col_overdue" to use the single-word, standardized
UI wording "Overdue" instead of "Over Due"; locate the <string
name="feature_loan_general_summary_col_overdue"> entry and replace its text
value so it matches other occurrences in the file and UI terminology.
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80fb8d49-e750-4600-a122-a28a0c3d2946
📒 Files selected for processing (4)
feature/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/navigation/LoanNavigation.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt
niyajali
left a comment
niyajali
left a comment
There was a problem hiding this comment.
check out mifos-pay project for referrence and update the PR accordingly with the required changes
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralScreen.kt (1)
432-449: Use resource strings in preview headings to avoid label drift.Preview labels are hardcoded (
"PERFORMANCE HISTORY","LOAN SUMMARY","LOAN DETAILS"). Using the samestringResource(...)keys as runtime UI keeps previews aligned with localization/content updates.Suggested preview-only cleanup
- Text( - text = "PERFORMANCE HISTORY", + Text( + text = stringResource(Res.string.feature_loan_general_section_performance_history), style = KptTheme.typography.labelLarge, modifier = Modifier.padding(horizontal = KptTheme.spacing.md), ) @@ - Text( - text = "LOAN SUMMARY", + Text( + text = stringResource(Res.string.feature_loan_general_section_loan_summary), style = KptTheme.typography.labelLarge, modifier = Modifier.padding(horizontal = KptTheme.spacing.md), ) @@ - Text( - text = "LOAN DETAILS", + Text( + text = stringResource(Res.string.feature_loan_general_section_loan_details), style = KptTheme.typography.labelLarge, modifier = Modifier.padding(horizontal = KptTheme.spacing.md), )🤖 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/loanAccountGeneral/LoanAccountGeneralScreen.kt` around lines 432 - 449, Replace hardcoded preview headings with the same localized string resources used at runtime to prevent label drift; locate the Text composables that currently use literal strings "PERFORMANCE HISTORY", "LOAN SUMMARY", and "LOAN DETAILS" and swap them to call stringResource(...) with the existing resource keys used by the runtime UI (the Text usages surrounding PerformanceHistoryCard and LoanSummaryTable), ensuring the preview uses the same keys so localization changes reflect in previews.
🤖 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/loanAccountGeneral/LoanAccountGeneralViewModel.kt`:
- Around line 103-105: The fields currency.code and currency.decimalPlaces on
the loan object are nullable but are read into currencyCode and maxDigits
without fallbacks; update the LoanAccountGeneralViewModel.kt assignment for
currencyCode and maxDigits (where loan.currency.code and
loan.currency.decimalPlaces are read) to provide safe defaults (e.g., "N/A" for
code and 2 for decimal places) before any CurrencyFormatter.format() calls so
downstream formatting uses consistent non-null values.
- Around line 108-117: The calls to DateHelper.getDateAsString in
LoanAccountGeneralViewModel (for maturityDate and disbursementDate) may receive
lists with fewer than 3 elements (e.g., after
actualDisbursementDate.filterNotNull()) and can throw IndexOutOfBoundsException;
before calling DateHelper.getDateAsString on expectedMaturityDate and
actualDisbursementDate.filterNotNull(), guard the list length (or normalize it)
— if the list size is less than 3 return a safe default (empty string) or
pad/validate the list to 3 elements; update the maturityDate and
disbursementDate assignments to check size() or use a small helper that returns
"" when the date parts list is invalid to avoid crashing the screen.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralScreen.kt`:
- Around line 432-449: Replace hardcoded preview headings with the same
localized string resources used at runtime to prevent label drift; locate the
Text composables that currently use literal strings "PERFORMANCE HISTORY", "LOAN
SUMMARY", and "LOAN DETAILS" and swap them to call stringResource(...) with the
existing resource keys used by the runtime UI (the Text usages surrounding
PerformanceHistoryCard and LoanSummaryTable), ensuring the preview uses the same
keys so localization changes reflect in previews.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f77c7a7-49ed-4d0b-bcab-334c67fce3b3
📒 Files selected for processing (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralViewModel.kt
|
@amanna13 The UI doesn't seem to be aligned with the UI of the Client Flow like typography. Please review. Performance history is a section and not the title of the screen. It should be Loan Account General. The mockup in ticket says General. However, we should add "Loan Account" as prefix since the Client Flow also has "General" screen as well. You may take a look at the client flow's General screen ensure consistency. 
|
|
The table components are not being handled properly, please review. Like the "G" of Outstanding appears in the second line. Also the paddings on left and right end seem to be different. Extreme Left: 
Extreme Right: 
|
There was a problem hiding this comment.
♻️ Duplicate comments (3)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralViewModel.kt (3)
111-112:⚠️ Potential issue | 🟡 MinorAdd null-safety fallbacks for
currencyCodeandmaxDigits.The fields
loan.currency.codeandloan.currency.decimalPlacesare nullable, but are used directly in allCurrencyFormatter.format()calls throughout lines 145-208. Provide safe defaults to ensure consistent formatting.🛡️ Suggested fix
private suspend fun fillGeneralState(loan: LoanWithAssociationsEntity) { - val currencyCode = loan.currency.code - val maxDigits = loan.currency.decimalPlaces + val currencyCode = loan.currency.code ?: "$" + val maxDigits = loan.currency.decimalPlaces ?: 2 val summary = loan.summaryBased on learnings: When a Kotlin function parameter is nullable and downstream calls require a non-null value, add null-safety handling with a safe default.
🤖 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/loanAccountGeneral/LoanAccountGeneralViewModel.kt` around lines 111 - 112, The variables currencyCode and maxDigits in LoanAccountGeneralViewModel are nullable (loan.currency.code and loan.currency.decimalPlaces) but are passed directly into CurrencyFormatter.format calls; add null-safety fallbacks (e.g., currencyCode ?: "" and maxDigits ?: 0 or another appropriate default) where these values are assigned or right before each CurrencyFormatter.format invocation so all format calls use non-null arguments and avoid NPEs.
115-127:⚠️ Potential issue | 🟠 MajorGuard date lists before calling
DateHelper.getDateAsString.At lines 117 and 124,
DateHelper.getDateAsString(...)is called without ensuring the list has at least 3 elements. ForactualDisbursementDate,filterNotNull()can reduce the list size below 3, causingIndexOutOfBoundsException.🛡️ Safer date mapping
val expectedMaturityDate = loan.timeline.expectedMaturityDate - val maturityDate = if (!expectedMaturityDate.isNullOrEmpty()) { - DateHelper.getDateAsString(expectedMaturityDate) - } else { - "" - } + val maturityDate = expectedMaturityDate + ?.takeIf { it.size >= 3 } + ?.let { runCatching { DateHelper.getDateAsString(it) }.getOrNull() } + .orEmpty() val actualDisbursementDate = loan.timeline.actualDisbursementDate - val disbursementDate = if (!actualDisbursementDate.isNullOrEmpty()) { - DateHelper.getDateAsString(actualDisbursementDate.filterNotNull()) - } else { - "" - } + val disbursementDate = actualDisbursementDate + ?.filterNotNull() + ?.takeIf { it.size >= 3 } + ?.let { runCatching { DateHelper.getDateAsString(it) }.getOrNull() } + .orEmpty()🤖 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/loanAccountGeneral/LoanAccountGeneralViewModel.kt` around lines 115 - 127, The code calls DateHelper.getDateAsString on expectedMaturityDate and actualDisbursementDate without ensuring the underlying lists contain the expected 3 elements, which can throw IndexOutOfBoundsException (especially after actualDisbursementDate.filterNotNull()); update the mapping in LoanAccountGeneralViewModel so you first validate the list size (and after filterNotNull for actualDisbursementDate) has at least 3 elements before calling DateHelper.getDateAsString, otherwise return an empty string or a safe default; reference the symbols expectedMaturityDate, actualDisbursementDate, DateHelper.getDateAsString to locate and guard the calls.
145-147:⚠️ Potential issue | 🟠 Major
proposedAmountandapprovedAmountuse identical source field.Both
proposedAmount(line 145) andapprovedAmount(line 146) are populated fromloan.approvedPrincipal, which appears incorrect. Based on past discussion, clarify with the team whetherloan.principalshould map toproposedAmountor ifLoanWithAssociationsEntityneeds a newproposedPrincipalfield.🤖 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/loanAccountGeneral/LoanAccountGeneralViewModel.kt` around lines 145 - 147, proposedAmount and approvedAmount are both set from loan.approvedPrincipal; update the mapping so proposedAmount uses the intended source (likely loan.principal) or, if the domain requires a distinct proposed value, add a proposedPrincipal to LoanWithAssociationsEntity and map proposedAmount to that field; adjust the assignments in LoanAccountGeneralViewModel (the proposedAmount and approvedAmount assignments) and coordinate with the team if a new proposedPrincipal property must be introduced on LoanWithAssociationsEntity before changing the mapping.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralScreen.kt (2)
65-84:onActionlambda is unused inLoanAccountGeneralContent.The
onActionlambda created at line 72 is only passed toLoanAccountGeneralDialogsbut not toLoanAccountGeneralContent. This is fine for now since content doesn't need actions, but therememberblock adds unnecessary overhead given it's only used for dialogs.Consider simplifying:
♻️ Suggested simplification
`@Composable` internal fun LoanAccountGeneralScreen( navController: NavController, modifier: Modifier = Modifier, viewModel: LoanAccountGeneralViewModel = koinViewModel(), ) { val state by viewModel.stateFlow.collectAsStateWithLifecycle() - val onAction = remember(viewModel) { { action: LoanAccountGeneralAction -> viewModel.trySendAction(action) } } LoanAccountGeneralContent( state = state, navController = navController, modifier = modifier, ) LoanAccountGeneralDialogs( state = state, - onAction = onAction, + onAction = viewModel::trySendAction, ) }🤖 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/loanAccountGeneral/LoanAccountGeneralScreen.kt` around lines 65 - 84, The remembered onAction lambda in LoanAccountGeneralScreen is unused by LoanAccountGeneralContent and only passed to LoanAccountGeneralDialogs, so remove the unnecessary remember and local onAction variable and instead pass a direct lambda (or method reference) to LoanAccountGeneralDialogs; update LoanAccountGeneralScreen to call LoanAccountGeneralDialogs with a concise callback (e.g., { action -> viewModel.trySendAction(action) } or viewModel::trySendAction) and keep LoanAccountGeneralContent as-is.
197-214:TextStyledefault may lose typography properties.The default
textStyleat lines 202-205 only copiesfontStylefromKptTheme.typography.labelMedium, losing other properties likefontSize,fontWeight,letterSpacing, andlineHeight. This could result in inconsistent text rendering.♻️ Suggested fix
`@Composable` private fun PerformanceHistoryRow( label: String, value: String, modifier: Modifier = Modifier, - textStyle: TextStyle = TextStyle( - color = AppColors.customWhite, - fontStyle = KptTheme.typography.labelMedium.fontStyle, - ), + textStyle: TextStyle = KptTheme.typography.labelMedium.copy( + color = AppColors.customWhite, + ), ) {🤖 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/loanAccountGeneral/LoanAccountGeneralScreen.kt` around lines 197 - 214, The default textStyle for PerformanceHistoryRow currently constructs a new TextStyle with only fontStyle set, which discards other typography properties; change the default to derive from the theme by using KptTheme.typography.labelMedium and overriding only the color (and optionally fontStyle) — e.g. set the textStyle default to KptTheme.typography.labelMedium.copy(color = AppColors.customWhite) or KptTheme.typography.labelMedium.merge(TextStyle(color = AppColors.customWhite)) so that fontSize, fontWeight, letterSpacing, lineHeight, etc., are preserved for the Text composables in PerformanceHistoryRow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralViewModel.kt`:
- Around line 111-112: The variables currencyCode and maxDigits in
LoanAccountGeneralViewModel are nullable (loan.currency.code and
loan.currency.decimalPlaces) but are passed directly into
CurrencyFormatter.format calls; add null-safety fallbacks (e.g., currencyCode ?:
"" and maxDigits ?: 0 or another appropriate default) where these values are
assigned or right before each CurrencyFormatter.format invocation so all format
calls use non-null arguments and avoid NPEs.
- Around line 115-127: The code calls DateHelper.getDateAsString on
expectedMaturityDate and actualDisbursementDate without ensuring the underlying
lists contain the expected 3 elements, which can throw IndexOutOfBoundsException
(especially after actualDisbursementDate.filterNotNull()); update the mapping in
LoanAccountGeneralViewModel so you first validate the list size (and after
filterNotNull for actualDisbursementDate) has at least 3 elements before calling
DateHelper.getDateAsString, otherwise return an empty string or a safe default;
reference the symbols expectedMaturityDate, actualDisbursementDate,
DateHelper.getDateAsString to locate and guard the calls.
- Around line 145-147: proposedAmount and approvedAmount are both set from
loan.approvedPrincipal; update the mapping so proposedAmount uses the intended
source (likely loan.principal) or, if the domain requires a distinct proposed
value, add a proposedPrincipal to LoanWithAssociationsEntity and map
proposedAmount to that field; adjust the assignments in
LoanAccountGeneralViewModel (the proposedAmount and approvedAmount assignments)
and coordinate with the team if a new proposedPrincipal property must be
introduced on LoanWithAssociationsEntity before changing the mapping.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralScreen.kt`:
- Around line 65-84: The remembered onAction lambda in LoanAccountGeneralScreen
is unused by LoanAccountGeneralContent and only passed to
LoanAccountGeneralDialogs, so remove the unnecessary remember and local onAction
variable and instead pass a direct lambda (or method reference) to
LoanAccountGeneralDialogs; update LoanAccountGeneralScreen to call
LoanAccountGeneralDialogs with a concise callback (e.g., { action ->
viewModel.trySendAction(action) } or viewModel::trySendAction) and keep
LoanAccountGeneralContent as-is.
- Around line 197-214: The default textStyle for PerformanceHistoryRow currently
constructs a new TextStyle with only fontStyle set, which discards other
typography properties; change the default to derive from the theme by using
KptTheme.typography.labelMedium and overriding only the color (and optionally
fontStyle) — e.g. set the textStyle default to
KptTheme.typography.labelMedium.copy(color = AppColors.customWhite) or
KptTheme.typography.labelMedium.merge(TextStyle(color = AppColors.customWhite))
so that fontSize, fontWeight, letterSpacing, lineHeight, etc., are preserved for
the Text composables in PerformanceHistoryRow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47b32120-947c-4c4d-bf2f-8cd21c66eeaa
📒 Files selected for processing (3)
feature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountGeneral/LoanAccountGeneralViewModel.kt
|
@amanna13 Please update the screen recording if UI has changed after the last changes. |
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
Looks good to me. This can be merged.
| override fun getLoanById(loanId: Int): Flow<DataState<LoanWithAssociations?>> { | ||
| return dataManagerLoan.getLoanById(loanId) | ||
| .asDataStateFlow() | ||
| .map { response -> | ||
| when (response) { | ||
| is DataState.Loading -> DataState.Loading | ||
| is DataState.Error -> DataState.Error(response.exception, response.data?.toModel()) | ||
| is DataState.Success -> DataState.Success(response.data?.toModel()) | ||
| } | ||
| } | ||
| .flowOn(ioDispatcher) |
There was a problem hiding this comment.
should map before converting it to datastate flow. not after that
something like
override fun getLoanById(loanId: Int): Flow<DataState<LoanWithAssociations?>> {
return dataManagerLoan.getLoanById(loanId)::mapToModel
.asDataStateFlow()
.flowOn(ioDispatcher)There was a problem hiding this comment.
I checked the repo pattern across modules. The common convention here is to wrap with asDataStateFlow() first, and then map if needed.
There was a problem hiding this comment.
could you please tell which module you took reference from?
There was a problem hiding this comment.
Sorry for that, my mistake, I overlooked. I have fixed this now.
| data class LoanWithAssociations( | ||
| val id: Int = 0, | ||
| val accountNo: String = "", | ||
| val status: LoanStatus = LoanStatus(), | ||
| val clientId: Int = 0, | ||
| val clientName: String = "", | ||
| val clientOfficeId: Int = 0, | ||
| val loanProductId: Int = 0, | ||
| val loanProductName: String = "", | ||
| val loanProductDescription: String = "", | ||
| val fundId: Int = 0, | ||
| val fundName: String = "", | ||
| val loanPurposeId: Int = 0, | ||
| val loanPurposeName: String = "", | ||
| val loanOfficerId: Int = 0, | ||
| val loanOfficerName: String = "", | ||
| val loanType: LoanType = LoanType(), | ||
| val currency: SavingAccountCurrency = SavingAccountCurrency(), | ||
| val principal: Double = 0.0, | ||
| val approvedPrincipal: Double = 0.0, | ||
| val termFrequency: Int = 0, | ||
| val termPeriodFrequencyType: TermPeriodFrequencyType = TermPeriodFrequencyType(), | ||
| val numberOfRepayments: Int = 0, |
There was a problem hiding this comment.
are we supossed to have empty values?
There was a problem hiding this comment.
yes this kind of default-value pattern is used in other modules too.
@biplab1 Can you also please verify and confirm.
There was a problem hiding this comment.
@amanna13 I checked mifos-mobile, it has the fields nullable and I think they can be null and it should be decided in the presentation layer how the null values should be handled.
There was a problem hiding this comment.
yes this kind of default-value pattern is used in other modules too.
It should not have a default value in the first place. Default values should be either null, an empty list, or something similar.
I’m not sure which module you took this reference from, but please create a Jira ticket so we can address it later.
Also, try to take references from the KMP template instead of other modules, as it is almost always up to date and we generally follow its structure.
| @Parcelize | ||
| @Serializable | ||
| data class LoanType( | ||
| val id: Int? = null, | ||
| val code: String? = null, | ||
| val value: String? = null, | ||
| ) : Parcelable | ||
|
|
||
| @Parcelize | ||
| @Serializable | ||
| data class SavingAccountCurrency( | ||
| val id: Int = 0, | ||
| val code: String? = null, | ||
| val name: String? = null, | ||
| val decimalPlaces: Int? = null, | ||
| val inMultiplesOf: Int? = null, | ||
| val displaySymbol: String? = null, | ||
| val nameCode: String? = null, | ||
| val displayLabel: String? = null, | ||
| ) : Parcelable |
There was a problem hiding this comment.
do not put all the classes in a single file. Do it separtely.
create a package inside loan called loanwithAssociations and put all the files there.
| @Serializable | ||
| data class ActualDisbursementDate( | ||
| val loanId: Int? = null, | ||
| val year: Int? = null, | ||
| val month: Int? = null, | ||
| val date: Int? = null, | ||
| ) : Parcelable | ||
|
|
||
| @Parcelize | ||
| @Serializable | ||
| data class LoansAccountSummary( | ||
| val loanId: Int? = null, | ||
| val currency: SavingAccountCurrency? = null, |
| private fun observeNetwork() { | ||
| viewModelScope.launch { | ||
| networkMonitor.isOnline.collect { isConnected -> | ||
| mutableStateFlow.update { it.copy(networkConnection = isConnected) } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I dont see a valid reason to continuously observe network state. we should only check connectivity when making network requests....
Current behavior:
After the user fetches loan details, the data is displayed correctly. However, if the network connection is lost, all the displayed items disappear and a “no internet” screen is shown.
My question - why should users need an active internet connection to view or scroll through data that has already been fetched?>>
we are already showing a snackbar to indicate loss of connectivity, so this additional thing impacts user experience.
Only check the network state before making an API call and show a dialog /screen
There was a problem hiding this comment.
I’ve verified the codebase has mixed patterns: some screens observe continuously, while others do one-shot connectivity checks before making a API calls. For instance ClientProfileGeneralViewModel and some LoanAccounts screen observes continously
Why haven't we yet standardize across modules, is there any specific reason we are using different approaches ?
Though I agree totally to the fact that there's no need to continuosly check for network state and I'm doing the necessary changes.
There was a problem hiding this comment.
- I think most contributors implemented it by taking reference from Mifos Mobile, where we followed that approach.
- While reviewing PRs, I may have suggested to some contributors to use the one-shot approach, so they adopted that instead.
Hence, the difference.
Yeah, you are right about it. We should standardise it.
niyajali
left a comment
niyajali
left a comment
There was a problem hiding this comment.
and the networkMontior could be moved in to the repository itself, and you can check the status before performing any action on other methods
| private fun loadLoanById() { | ||
| loadJob?.cancel() | ||
| loadJob = viewModelScope.launch { | ||
| val isConnected = networkMonitor.isOnline.first() |
There was a problem hiding this comment.
use kotlin combine operator function to combine the two streams into a single stream, and that way it will refeed the data from both sources when back online
| details = listOf( | ||
| mapOf( | ||
| Res.string.feature_loan_general_detail_disbursement_date to disbursementDate, | ||
| Res.string.feature_loan_general_detail_loan_purpose to loanPurpose, | ||
| Res.string.feature_loan_general_detail_loan_officer to loanOfficer, | ||
| Res.string.feature_loan_general_detail_currency to currencyDisplay, | ||
| ), | ||
| mapOf( | ||
| Res.string.feature_loan_general_detail_proposed_amount to CurrencyFormatter.format(loan.approvedPrincipal, currencyCode, maxDigits), | ||
| Res.string.feature_loan_general_detail_approved_amount to CurrencyFormatter.format(loan.approvedPrincipal, currencyCode, maxDigits), | ||
| Res.string.feature_loan_general_detail_disbursed_amount to CurrencyFormatter.format(loan.principal, currencyCode, maxDigits), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
One of the issues of using a Map is it is unstructured, so type safety could be a problem.
Instead of Map, we can use,
a proper model:
data class DetailItem(
val label: StringResource,
val value: String
)or even better:
sealed interface LoanDetailItem
| proposedAmount = CurrencyFormatter.format(loan.approvedPrincipal, currencyCode, maxDigits), | ||
| approvedAmount = CurrencyFormatter.format(loan.approvedPrincipal, currencyCode, maxDigits), |
There was a problem hiding this comment.
How can both use the same field approvedPrincipal?
fix : merge conflicts resolved fix : merge conflicts resolved fix : merge conflicts resolved
fix : dto and mappers added
Kartikey15dem
force-pushed
the
feat/general-tab-loan-section
branch
from
05df700 to
92a8106
Compare
|
|
@Kartikey15dem I don't think this is the correct way to handle the requested changes. This PR shouldn't contain changes from other modules. We should preserve the older data classes for modules which are outside the scope of this PR, and introduce new data classes as requested for this module only. Later, other modules will be refactored in separate PRs. Please review. |
@biplab1 These changes came as a result of refactoring LoanWithAssociationsDto. That refactor introduced a series of dependent changes which were required to keep the app running correctly. |
7 participants
Fixes - Jira-#665
Screenshot [Updated]
595600271-d0770f03-972e-4c4e-8fc7-dca8a12e106e.webm
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit