feat(loan): Add missing repayment features to the Repayment Form - #2692
Open
Arinyadav1 wants to merge 3 commits into
Open
feat(loan): Add missing repayment features to the Repayment Form#2692Arinyadav1 wants to merge 3 commits into
Arinyadav1 wants to merge 3 commits into
Conversation
…anagement and improving UI feedback.
Summary by CodeRabbit
WalkthroughThis PR refactors loan repayment into an action-driven ViewModel with DataState-backed repository calls and penalty-selection UI, updates date formatting utility, adds payment-detail fields and strings, and bumps the iOS podspec while adding xcconfig/pod_target_xcconfig settings. ChangesLoan Repayment Feature Modernization — overviewA set of coordinated changes refactors repository contracts and implementations, updates the date helper, rewrites the ViewModel to an action/state machine, rebuilds the Compose screen into dialog/content components with expandable payment-details and penalties sections, and updates navigation/resources. Loan Repayment Feature Modernization
iOS CocoaPods Build Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.kt (1)
75-81:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Format mismatch causes parsing failure.
The call to
getDateAsString(integersOfDate.map { it.toInt() })on line 79 now uses the new default pattern"dd MMM yyyy"(month name), producing output like"15 Mar 2024". However,getFormatConverteron line 76 expectscurrentFormat = FULL_MONTH = "dd MM yyyy"(numeric month), and will attempt to parse"15 Mar 2024"as"dd MM yyyy"format. The parser will fail because"Mar"is not a valid numeric month.🐛 Proposed fix
Explicitly pass the
FULL_MONTHpattern to match the expected format:fun getDateAsStringFromListLong(integersOfDate: List<Long>, pattern: String): String { return getFormatConverter( currentFormat = FULL_MONTH, requiredFormat = pattern, - dateString = getDateAsString(integersOfDate.map { it.toInt() }), + dateString = getDateAsString(integersOfDate.map { it.toInt() }, FULL_MONTH), ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.kt` around lines 75 - 81, getDateAsStringFromListLong currently builds the input string with getDateAsString(which now defaults to "dd MMM yyyy") but then calls getFormatConverter(currentFormat = FULL_MONTH = "dd MM yyyy"), causing parse failure; fix by calling getDateAsString with the explicit FULL_MONTH pattern (i.e., pass FULL_MONTH into getDateAsString) so the produced date string matches the currentFormat expected by getFormatConverter, leaving the requiredFormat parameter as pattern.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt (1)
72-74: 💤 Low valueDirect method call from async handler violates internal reducer pattern.
Per MVI guidelines, async handlers should dispatch follow-up internal actions rather than directly calling other data-loading methods. This applies to both
getLoanRepaymentTemplate()(line 73) andgetLoanCharges()(line 133).Preferred pattern using internal actions
Add internal actions for triggering subsequent loads:
sealed interface Internal : LoanRepaymentAction { // existing actions... object LoadRepaymentTemplate : Internal object LoadLoanCharges : Internal }Then in
handleAction:is DataState.Success -> { mutableStateFlow.update { /* ... */ } sendAction(LoanRepaymentAction.Internal.LoadRepaymentTemplate) }And handle:
is LoanRepaymentAction.Internal.LoadRepaymentTemplate -> { getLoanRepaymentTemplate() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt` around lines 72 - 74, The async handler is directly calling getLoanRepaymentTemplate() and getLoanCharges(), violating the MVI reducer pattern; instead add two Internal actions (e.g., LoanRepaymentAction.Internal.LoadRepaymentTemplate and LoanRepaymentAction.Internal.LoadLoanCharges) to the sealed Internal interface, change the DataState.Success branches in handleAction to dispatch those internal actions via sendAction(...) rather than calling the loader methods, and add corresponding handleAction cases that invoke getLoanRepaymentTemplate() and getLoanCharges() respectively.cmp-shared/cmp_shared.podspec (1)
25-27: ⚖️ Poor tradeoffENABLE_USER_SCRIPT_SANDBOXING=NO matches Kotlin’s CocoaPods guidance—no major security issue
Kotlin Multiplatform’s CocoaPods Xcode setup explicitly instructs disabling “User Script Sandboxing” (set
ENABLE_USER_SCRIPT_SANDBOXING=NO) for the iOS app target/build settings used for Kotlin→Pod integration, since the Run Script phases can fail under sandboxing. See: kotlinlang.org
Given that, the change in cmp-shared/cmp_shared.podspec is consistent with required integration behavior; the review should instead encourage leaving a brief in-repo justification/comment for maintainers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmp-shared/cmp_shared.podspec` around lines 25 - 27, The podspec change setting spec.xcconfig with 'ENABLE_USER_SCRIPT_SANDBOXING' => 'NO' is valid for Kotlin Multiplatform CocoaPods integration but needs an in-repo justification; add a short comment near the spec.xcconfig entry (or in the podspec header/README) explaining that ENABLE_USER_SCRIPT_SANDBOXING is intentionally disabled to support Kotlin→Pod Run Script phases per Kotlin Multiplatform CocoaPods guidance, and include a link to the kotlinlang.org doc for future maintainers and reviewers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmp-shared/cmp_shared.podspec`:
- Line 53: The podspec sets spec.resources using Windows-style backslashes which
will break CocoaPods on macOS/Linux; update the spec.resources assignment (the
spec.resources = ['build\compose\cocoapods\compose-resources'] line) to use
POSIX forward slashes (e.g., 'build/compose/cocoapods/compose-resources') so
resource resolution works on iOS builds.
In
`@core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.kt`:
- Around line 53-57: The method getDatabaseLoanRepaymentByLoanId is incorrectly
wrapped with networkMonitor.withNetworkCheck which blocks offline access;
instead call
dataManagerLoan.getDatabaseLoanRepaymentByLoanId(loanId).asDataStateFlow()
directly and apply .flowOn(dispatcher.io) to that Flow, removing the
networkMonitor.withNetworkCheck wrapper so local DB reads succeed when offline;
update the implementation in LoanRepaymentRepositoryImp to reference
getDatabaseLoanRepaymentByLoanId,
dataManagerLoan.getDatabaseLoanRepaymentByLoanId, and dispatcher.io accordingly.
In
`@core/database/src/commonMain/kotlin/com/mifos/room/entities/accounts/loans/LoanRepaymentRequestEntity.kt`:
- Around line 59-60: LoanRepaymentRequestEntity.externalId was added but the DB
version (MifosDatabase.VERSION) and builders (e.g., DatabaseModule.android.kt)
weren’t updated, so update the Room schema strategy: increment
MifosDatabase.VERSION and either add a Migration instance via addMigrations(...)
in the Room.databaseBuilder call, enable autoMigrations for MifosDatabase
(autoMigrations = [...]) and annotate entities if needed, or explicitly opt into
destructive migration on upgrade (fallbackToDestructiveMigration()) in the
database builders; ensure the change references MifosDatabase.VERSION and the
LoanRepaymentRequestEntity class so upgrades won't break at runtime.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt`:
- Around line 232-245: In LoanRepaymentScreen's MifosTextFieldDropdown for
payment type (inside the composable where state.paymentTypeIdIndex and
state.loanRepaymentTemplate?.paymentTypeOptions are used), replace the incorrect
label Res.string.loan_officer with the correct resource
Res.string.feature_loan_payment_type so the dropdown label matches the
paymentTypeOptions selection.
- Around line 129-135: LoanRepaymentContent currently accepts NavController
which violates Layer 2 rules; remove the navController parameter from the
LoanRepaymentContent signature and either (A) move the MifosBreadcrumbNavBar
invocation into the parent LoanRepaymentScreen so navigation stays in Layer 1,
or (B) change MifosBreadcrumbNavBar to be driven by state and emit a
back/navigation event via the existing onAction (LoanRepaymentAction) so
LoanRepaymentContent only receives state and onAction; update any usage of
MifosBreadcrumbNavBar inside LoanRepaymentContent to use the chosen approach and
ensure LoanRepaymentScreen handles the actual NavController.navigation calls.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt`:
- Around line 315-324: The UpdatePaymentTypeIdIndex branch in
LoanRepaymentViewModel uses direct list access
state.loanRepaymentTemplate?.paymentTypeOptions?.get(index) which can throw
IndexOutOfBoundsException; change the lookup to a safe call using
getOrNull(index) and derive paymentTypeId only if the returned element is
non-null (falling back to null or clearing paymentTypeId when out of range),
keeping the rest of the mutableStateFlow.update logic and the
LoanRepaymentAction.UpdatePaymentTypeIdIndex handler unchanged.
- Around line 495-496: The state has an inconsistent default where
selectAllPenalties is true but selectedPenaltyIds is empty; update the initial
state in LoanRepaymentViewModel (the data class/property that defines
selectAllPenalties and selectedPenaltyIds) to set selectAllPenalties = false (or
alternatively document the intended behavior) so the defaults reflect no
selection until charges load, ensuring UI and logic like any code referring to
selectAllPenalties or selectedPenaltyIds behave predictably.
---
Outside diff comments:
In `@core/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.kt`:
- Around line 75-81: getDateAsStringFromListLong currently builds the input
string with getDateAsString(which now defaults to "dd MMM yyyy") but then calls
getFormatConverter(currentFormat = FULL_MONTH = "dd MM yyyy"), causing parse
failure; fix by calling getDateAsString with the explicit FULL_MONTH pattern
(i.e., pass FULL_MONTH into getDateAsString) so the produced date string matches
the currentFormat expected by getFormatConverter, leaving the requiredFormat
parameter as pattern.
---
Nitpick comments:
In `@cmp-shared/cmp_shared.podspec`:
- Around line 25-27: The podspec change setting spec.xcconfig with
'ENABLE_USER_SCRIPT_SANDBOXING' => 'NO' is valid for Kotlin Multiplatform
CocoaPods integration but needs an in-repo justification; add a short comment
near the spec.xcconfig entry (or in the podspec header/README) explaining that
ENABLE_USER_SCRIPT_SANDBOXING is intentionally disabled to support Kotlin→Pod
Run Script phases per Kotlin Multiplatform CocoaPods guidance, and include a
link to the kotlinlang.org doc for future maintainers and reviewers.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt`:
- Around line 72-74: The async handler is directly calling
getLoanRepaymentTemplate() and getLoanCharges(), violating the MVI reducer
pattern; instead add two Internal actions (e.g.,
LoanRepaymentAction.Internal.LoadRepaymentTemplate and
LoanRepaymentAction.Internal.LoadLoanCharges) to the sealed Internal interface,
change the DataState.Success branches in handleAction to dispatch those internal
actions via sendAction(...) rather than calling the loader methods, and add
corresponding handleAction cases that invoke getLoanRepaymentTemplate() and
getLoanCharges() respectively.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 909d66fb-23fd-44a8-9304-0d7cf94443a7
📒 Files selected for processing (11)
cmp-shared/cmp_shared.podspeccore/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanRepaymentRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/accounts/loans/LoanRepaymentRequestEntity.ktfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentUiState.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
💤 Files with no reviewable changes (1)
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentUiState.kt
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🧰 Additional context used
📓 Path-based instructions (7)
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: Additional Code Review Guidelines:
- Null Safety & Stability
- Avoid using
!!operator- Handle null cases explicitly using safe calls or proper state handling
- Do not assume values are always non-null without guarantees
- Architecture Boundaries
- ViewModel must not depend on specific network/library implementations
- Ensure proper separation between data, domain, and presentation layers
- Do not format data (currency, dates, calculations) inside the UI layer
- All formatting must be handled in the ViewModel and exposed via state (e.g., StateFlow)
- Performance Considerations
- Avoid unnecessary recompositions in Compose
- Do not attach heavy logic to frequently changing states (e.g., scrollState)
- Prefer lifting state up instead of recomputing in child composables
- Compose & Navigation Best Practices
- NEVER trigger navigation functions or side-effects directly during composition
- Always wrap navigation calls inside
LaunchedEffectorEventsEffectto avoid repeated execution on recomposition- Avoid triggering intensive side-effects during recomposition
- Navigation routes must be type-safe.
- Ensure all route classes or objects used for navigation are annotated with
@Serializable.
- UI Structure
- Dialogs must be separated into their own composables
- Do not embed dialogs inline within complex main screens
- Localization Consistency
- Ensure all supported languages are updated consistently across modules
- Verify translations exist for newly added UI strings
- Code Cleanliness
- Avoid unnecessary inline comments unless critical
- Remove leftover debug or commented code
- Focus on correctness, readability, and maintainability over cosmetic nitpicks.
- Avoid reviewing README, config, or asset files.
- Prioritize identifying bugs, performance issues, and architectural concerns.
- Naming & Intent Rules:
- Follow the official Kotlin Coding Conventions:
https://kotlinlang.org/docs/coding-conventions.html- Use self-explanatory ...
Files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/accounts/loans/LoanRepaymentRequestEntity.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreenRoute.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanRepaymentRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.ktcore/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
**/{core-base,core}/**/*.kt
⚙️ CodeRabbit configuration file
**/{core-base,core}/**/*.kt: Critical Module Change Detection:
- Changes in
core-basemodule must be treated as high-impact.- Flag any PR that modifies files inside
core-basefor careful review.- Verify that modifications in
core-baseare necessary and minimal.
Output:- Clearly highlight that
core-baseis a shared foundational module and requires extra review attention.
Files:
core/database/src/commonMain/kotlin/com/mifos/room/entities/accounts/loans/LoanRepaymentRequestEntity.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanRepaymentRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.ktcore/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.kt
**/composeResources/values*/strings.xml
⚙️ CodeRabbit configuration file
**/composeResources/values*/strings.xml: String resource conventions:Naming:
- All keys must follow:
feature_{feature_name}_{ui_text_in_snake_case}Examples:
"In Advance" → feature_loan_in_advance
"Outstanding" → feature_loan_outstanding
- Avoid generic names like: title, text1, label
- The suffix should be a short, readable representation of the UI text
- Avoid multiple keys representing the same UI text
- Keys must be lowercase and use snake_case
Flag:
- Incorrect naming pattern
- Generic or unclear key names
- Duplicate keys for same UI text
Files:
feature/loan/src/commonMain/composeResources/values/strings.xml
**/data/repository/**/*.kt
⚙️ CodeRabbit configuration file
**/data/repository/**/*.kt: Repository Interface Guidelines:
- One-shot operations MUST use
suspend funand returnDataState<T>. MUST NOT use Flow.- Streaming operations MUST return
Flow<DataState<T>>and MUST NOT usesuspend.- Repository interfaces must consistently return Domain models (from :core:model) wrapped in
DataStateorFlow.- Flag: Any interface method that returns a DTO or an Entity.
Files:
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanRepaymentRepository.kt
**/data/repositoryImp/**/*.kt
⚙️ CodeRabbit configuration file
**/data/repositoryImp/**/*.kt: Repository Implementation Guidelines:
- MUST convert
T->DataState<T>for one-shot APIs, andFlow<T>->Flow<DataState<T>>using.asDataStateFlow().- MUST perform network availability checks, handle exceptions, and apply appropriate dispatchers.
- Repositories act as the boundary translator. They MUST use mappers to convert DTOs (from network) and Entities (from database) into Domain Models (from :core:model) before returning data.
- Flag: If a repository implementation returns a raw DTO or Entity directly to the caller instead of a Domain model.
Files:
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.kt
**/*ViewModel.kt
⚙️ CodeRabbit configuration file
**/*ViewModel.kt: MVI architecture rules:
- All new features must follow MVI
- ViewModel must extend BaseViewModel
- The ViewModel MUST maintain a single UI state (e.g., a single StateFlow)
instead of multiple separate state variables.Required:
- Use *State, *Event, *Action
- Naming must be consistent (FeatureViewModel, FeatureState, FeatureEvent, FeatureAction)
- Follow unidirectional flow:
Action → ViewModel → State → UIInternal reducer/action architecture rules:
Async operations MUST NOT directly mutate UI state repeatedly inside
Flow collectors, suspend callbacks, or repository result handlers.Repository/network/database results MUST be converted into internal
actions usingsendAction(...).
handleAction(...)must act as the primary reducer responsible for:
- state mutation
- reducer-style state transitions
- triggering follow-up actions
Large async methods must be split into:
- async collection layer
- internal action dispatching
- reducer/state handling
Avoid directly calling another business/data-loading method from
repository collectors or async callbacks.
Prefer dispatching follow-up internal actions instead.Preferred pattern:
repository result
-> sendAction(...)
-> handleAction(...)
-> mutableStateFlow.update { ... }Anti-pattern examples:
mutableStateFlow.update { ... } inside collect { }
fetchX() -> directly calls fetchY() inside async collector
large methods mixing:
- collection
- state mutation
- navigation
- business chainingFlag:
- Multiple
mutableStateFlow.update {}calls inside collect { }- Direct state mutation inside async repository callbacks
- Async methods performing both collection and reducer logic
- Direct business-flow chaining from async collectors
- Missing internal reducer actions for async results
- Missing *State / *Event / *Action
- ViewModel not...
Files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt
**/*Screen.kt
⚙️ CodeRabbit configuration file
**/*Screen.kt: Screen architecture rules:Each screen must follow a 2-layer structure:
Layer 1 (Entry/Route Composable):
- Function Name: Typically
*ScreenRouteor the entry-point composable.- Responsibilities: Inject ViewModel (e.g.,
koinViewModel), take navigation lambdas, collect theStateFlow, and handle ViewModel events.- Logic: Should only handle ViewModel interaction, state collection, and triggering navigation in response to ViewModel events.
Layer 2 (Stateful/Content Composable —
*Screenor*ScreenContent):
- Parameters: MUST only take
state(the UI state object) and a singleonActionlambda (e.g.,onAction: (FeatureAction) -> Unit).- Responsibilities: Render the UI based strictly on the provided
state.- Rule: MUST NOT pass multiple separate lambda functions for different UI interactions; consolidate them into the single
onAction.- Constraint: Must NOT contain any business logic or ViewModel/Navigation references.
Internal/private helper composables (e.g., dialogs, sections, sub-components):
- These are NOT subject to the single
onActionrule.- They may accept specific, focused lambdas (e.g.,
onRetry: () -> Unit) or a narrowedonActionas appropriate.- However, they must NOT be passed the ViewModel or navigation controllers directly.
UI consistency:
- Avoid hardcoded values (dp, sp, padding, fontSize, colors)
- Use DesignToken, KptTheme, AppColors, MifosTypography for spacing, typography, and colors
Code quality:
- Keep Composables small and readable
- Avoid deeply nested UI
Flag:
- Missing Layer 2 (
*Screen/*ScreenContent) separation from the entry-point composable- Layer 2 (
*Screen/*ScreenContent) receiving multiple separate lambdas instead of a singleonAction- UI logic inside the entry-point composable
- Business logic inside any Composable
- Hardcoded strings instead of using string resources
- Hardcoded dp/sp values
- Direct styling instead of using DesignToken or KptTheme
Files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
🧠 Learnings (3)
📚 Learning: 2026-02-06T13:15:16.968Z
Learnt from: sahilshivekar
Repo: openMF/android-client PR: 2603
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt:43-106
Timestamp: 2026-02-06T13:15:16.968Z
Learning: Guideline: When a Kotlin function parameter is nullable (e.g., balance: Double?, currencyCode: String?, maximumFractionDigits: Int?) and downstream calls require a non-null value, add null-safety handling in all implementations. Specifically, avoid calling Currency.getInstance(currencyCode) with a possibly null currencyCode; provide a safe default (e.g., currencyCode ?: "$") or validate before use. Ensure all platform targets (Android/Desktop/Native) follow consistent null handling, and consider centralizing currencyCode normalization in the common layer if feasible. Add tests covering null currencyCode to prevent NPEs.
Applied to files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/accounts/loans/LoanRepaymentRequestEntity.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreenRoute.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanRepaymentRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.ktcore/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
📚 Learning: 2026-04-01T05:03:14.323Z
Learnt from: kartikey004
Repo: openMF/mifos-x-field-officer-app PR: 2659
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanDisbursement/LoanAccountDisbursementScreen.kt:190-195
Timestamp: 2026-04-01T05:03:14.323Z
Learning: In this repo, existing `SelectableDates` / `SelectableDates.isSelectableDate(utcTimeMillis: Long)` implementations use `Clock.System.now().toEpochMilliseconds()` (UTC epoch millis) for date-boundary checks. During PR reviews, do not flag these checks for not using a timezone-aware `LocalDate` start-of-day approach; treat it as an established project-wide pattern. If a change is desired, handle it as a coordinated project-wide improvement rather than as a per-PR review issue.
Applied to files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/accounts/loans/LoanRepaymentRequestEntity.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreenRoute.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanRepaymentRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.ktcore/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
📚 Learning: 2026-03-16T11:58:32.496Z
Learnt from: sahilshivekar
Repo: openMF/mifos-x-field-officer-app PR: 2647
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt:96-96
Timestamp: 2026-03-16T11:58:32.496Z
Learning: In feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt (Kotlin/Compose Multiplatform), navigateToDashboard uses a direct method reference navController::navigateToLoanDashboardScreen without an ID guard. This is intentional: LoanAccountProfileScreen always provides a valid loanId, and LoanDashboardScreen handles invalid/missing data by showing a 'Failed to fetch Loan details' error state. Do not flag the absence of an ID guard here in future reviews.
Applied to files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
🔇 Additional comments (16)
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.kt (1)
56-73: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanRepaymentRepository.kt (1)
25-28: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.kt (1)
35-51: LGTM!feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt (2)
33-38: LGTM!Also applies to: 250-264, 467-473, 506-530
435-445:ToggleAllPenalty:ChargesEntity.idis non-null, so current Set building is safe
ChargesEntity.idis declared asval id: Int = 0(non-nullInt) in ChargesEntity.kt, sostate.loanCharges.map { it.id }.toSet()won’t introduce nulls—nomapNotNullneeded.cmp-shared/cmp_shared.podspec (2)
3-3: LGTM!
29-32: LGTM!feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt (6)
79-102: LGTM!
104-127: LGTM!
136-168: LGTM!
169-230: LGTM!Also applies to: 246-288
290-358: LGTM!
360-435: LGTM!feature/loan/src/commonMain/composeResources/values/strings.xml (1)
478-490: LGTM!feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreenRoute.kt (1)
27-37: LGTM!feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt (1)
74-77: LGTM!
…epaymentScreen`.
- **Data**: Simplify `getDatabaseLoanRepaymentByLoanId` in `LoanRepaymentRepositoryImp` by removing unnecessary network check and dispatcher management.
- **UI**:
- Update the payment type selection label in `LoanRepaymentScreen` to use the correct string resource.
- Add `@OptIn` annotations for `ExperimentalMaterial3Api` and `ExperimentalTime` in `LoanRepaymentContent`.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt (2)
195-203:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
"null"string displayed when template data is missing.Calling
.toString()on nullable values renders the literal string"null"in the UI whenloanRepaymentTemplateor its portions are null.Proposed fix — use safe defaults
MifosDefaultListingComponentFromStringResources( data = mapOf( - Res.string.feature_loan_principal to state.loanRepaymentTemplate?.principalPortion.toString(), - Res.string.feature_loan_table_header_interest to state.loanRepaymentTemplate?.interestPortion.toString(), - Res.string.feature_loan_table_header_fees to state.loanRepaymentTemplate?.feeChargesPortion.toString(), - Res.string.feature_loan_penalties_short to state.penaltyAmount.toString(), + Res.string.feature_loan_principal to (state.loanRepaymentTemplate?.principalPortion?.toString() ?: "—"), + Res.string.feature_loan_table_header_interest to (state.loanRepaymentTemplate?.interestPortion?.toString() ?: "—"), + Res.string.feature_loan_table_header_fees to (state.loanRepaymentTemplate?.feeChargesPortion?.toString() ?: "—"), + Res.string.feature_loan_penalties_short to (state.penaltyAmount?.toString() ?: "—"), ), verticalArrangement = Arrangement.spacedBy(KptTheme.spacing.sm), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt` around lines 195 - 203, MifosDefaultListingComponentFromStringResources is displaying the literal "null" because the code calls .toString() on nullable fields (state.loanRepaymentTemplate and its principalPortion, interestPortion, feeChargesPortion, penaltyAmount); update the map values to use safe calls and defaults (e.g., state.loanRepaymentTemplate?.principalPortion ?: defaultValue or state.loanRepaymentTemplate?.principalPortion?.toString() ?: "") so nulls render as a sensible default/empty string; change the entries for principalPortion, interestPortion, feeChargesPortion and penaltyAmount accordingly.
235-248:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential
IndexOutOfBoundsExceptionif index becomes stale.If
paymentTypeOptionschanges (e.g., template reloads) whilepaymentTypeIdIndexretains a now-invalid value,get(state.paymentTypeIdIndex)throws. Use safe access.Proposed fix
MifosTextFieldDropdown( value = if (state.paymentTypeIdIndex == null) { "" } else { - state.loanRepaymentTemplate?.paymentTypeOptions?.get(state.paymentTypeIdIndex)?.name.toString() + state.loanRepaymentTemplate?.paymentTypeOptions?.getOrNull(state.paymentTypeIdIndex)?.name.orEmpty() },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt` around lines 235 - 248, The dropdown value access can throw IndexOutOfBounds when paymentTypeOptions changes while state.paymentTypeIdIndex remains stale; update the MifosTextFieldDropdown value logic in LoanRepaymentScreen to use safe/bounds-checked access (check state.paymentTypeIdIndex != null and that it is within 0 until state.loanRepaymentTemplate?.paymentTypeOptions?.size) before calling .get(...).name, fall back to "" if out of range, and optionally reset the index via onAction(LoanRepaymentAction.UpdatePaymentTypeIdIndex(null)) when you detect an invalid index so the selected index stays consistent with the mapped options.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt`:
- Around line 195-203: MifosDefaultListingComponentFromStringResources is
displaying the literal "null" because the code calls .toString() on nullable
fields (state.loanRepaymentTemplate and its principalPortion, interestPortion,
feeChargesPortion, penaltyAmount); update the map values to use safe calls and
defaults (e.g., state.loanRepaymentTemplate?.principalPortion ?: defaultValue or
state.loanRepaymentTemplate?.principalPortion?.toString() ?: "") so nulls render
as a sensible default/empty string; change the entries for principalPortion,
interestPortion, feeChargesPortion and penaltyAmount accordingly.
- Around line 235-248: The dropdown value access can throw IndexOutOfBounds when
paymentTypeOptions changes while state.paymentTypeIdIndex remains stale; update
the MifosTextFieldDropdown value logic in LoanRepaymentScreen to use
safe/bounds-checked access (check state.paymentTypeIdIndex != null and that it
is within 0 until state.loanRepaymentTemplate?.paymentTypeOptions?.size) before
calling .get(...).name, fall back to "" if out of range, and optionally reset
the index via onAction(LoanRepaymentAction.UpdatePaymentTypeIdIndex(null)) when
you detect an invalid index so the selected index stays consistent with the
mapped options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fdc06124-9df7-43a5-a974-76d63b7ba67f
📒 Files selected for processing (3)
cmp-shared/cmp_shared.podspeccore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🧰 Additional context used
📓 Path-based instructions (4)
**/data/repositoryImp/**/*.kt
⚙️ CodeRabbit configuration file
**/data/repositoryImp/**/*.kt: Repository Implementation Guidelines:
- MUST convert
T->DataState<T>for one-shot APIs, andFlow<T>->Flow<DataState<T>>using.asDataStateFlow().- MUST perform network availability checks, handle exceptions, and apply appropriate dispatchers.
- Repositories act as the boundary translator. They MUST use mappers to convert DTOs (from network) and Entities (from database) into Domain Models (from :core:model) before returning data.
- Flag: If a repository implementation returns a raw DTO or Entity directly to the caller instead of a Domain model.
Files:
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.kt
**/{core-base,core}/**/*.kt
⚙️ CodeRabbit configuration file
**/{core-base,core}/**/*.kt: Critical Module Change Detection:
- Changes in
core-basemodule must be treated as high-impact.- Flag any PR that modifies files inside
core-basefor careful review.- Verify that modifications in
core-baseare necessary and minimal.
Output:- Clearly highlight that
core-baseis a shared foundational module and requires extra review attention.
Files:
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.kt
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: Additional Code Review Guidelines:
- Null Safety & Stability
- Avoid using
!!operator- Handle null cases explicitly using safe calls or proper state handling
- Do not assume values are always non-null without guarantees
- Architecture Boundaries
- ViewModel must not depend on specific network/library implementations
- Ensure proper separation between data, domain, and presentation layers
- Do not format data (currency, dates, calculations) inside the UI layer
- All formatting must be handled in the ViewModel and exposed via state (e.g., StateFlow)
- Performance Considerations
- Avoid unnecessary recompositions in Compose
- Do not attach heavy logic to frequently changing states (e.g., scrollState)
- Prefer lifting state up instead of recomputing in child composables
- Compose & Navigation Best Practices
- NEVER trigger navigation functions or side-effects directly during composition
- Always wrap navigation calls inside
LaunchedEffectorEventsEffectto avoid repeated execution on recomposition- Avoid triggering intensive side-effects during recomposition
- Navigation routes must be type-safe.
- Ensure all route classes or objects used for navigation are annotated with
@Serializable.
- UI Structure
- Dialogs must be separated into their own composables
- Do not embed dialogs inline within complex main screens
- Localization Consistency
- Ensure all supported languages are updated consistently across modules
- Verify translations exist for newly added UI strings
- Code Cleanliness
- Avoid unnecessary inline comments unless critical
- Remove leftover debug or commented code
- Focus on correctness, readability, and maintainability over cosmetic nitpicks.
- Avoid reviewing README, config, or asset files.
- Prioritize identifying bugs, performance issues, and architectural concerns.
- Naming & Intent Rules:
- Follow the official Kotlin Coding Conventions:
https://kotlinlang.org/docs/coding-conventions.html- Use self-explanatory ...
Files:
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
**/*Screen.kt
⚙️ CodeRabbit configuration file
**/*Screen.kt: Screen architecture rules:Each screen must follow a 2-layer structure:
Layer 1 (Entry/Route Composable):
- Function Name: Typically
*ScreenRouteor the entry-point composable.- Responsibilities: Inject ViewModel (e.g.,
koinViewModel), take navigation lambdas, collect theStateFlow, and handle ViewModel events.- Logic: Should only handle ViewModel interaction, state collection, and triggering navigation in response to ViewModel events.
Layer 2 (Stateful/Content Composable —
*Screenor*ScreenContent):
- Parameters: MUST only take
state(the UI state object) and a singleonActionlambda (e.g.,onAction: (FeatureAction) -> Unit).- Responsibilities: Render the UI based strictly on the provided
state.- Rule: MUST NOT pass multiple separate lambda functions for different UI interactions; consolidate them into the single
onAction.- Constraint: Must NOT contain any business logic or ViewModel/Navigation references.
Internal/private helper composables (e.g., dialogs, sections, sub-components):
- These are NOT subject to the single
onActionrule.- They may accept specific, focused lambdas (e.g.,
onRetry: () -> Unit) or a narrowedonActionas appropriate.- However, they must NOT be passed the ViewModel or navigation controllers directly.
UI consistency:
- Avoid hardcoded values (dp, sp, padding, fontSize, colors)
- Use DesignToken, KptTheme, AppColors, MifosTypography for spacing, typography, and colors
Code quality:
- Keep Composables small and readable
- Avoid deeply nested UI
Flag:
- Missing Layer 2 (
*Screen/*ScreenContent) separation from the entry-point composable- Layer 2 (
*Screen/*ScreenContent) receiving multiple separate lambdas instead of a singleonAction- UI logic inside the entry-point composable
- Business logic inside any Composable
- Hardcoded strings instead of using string resources
- Hardcoded dp/sp values
- Direct styling instead of using DesignToken or KptTheme
Files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
🧠 Learnings (2)
📚 Learning: 2026-02-06T13:15:16.968Z
Learnt from: sahilshivekar
Repo: openMF/android-client PR: 2603
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt:43-106
Timestamp: 2026-02-06T13:15:16.968Z
Learning: Guideline: When a Kotlin function parameter is nullable (e.g., balance: Double?, currencyCode: String?, maximumFractionDigits: Int?) and downstream calls require a non-null value, add null-safety handling in all implementations. Specifically, avoid calling Currency.getInstance(currencyCode) with a possibly null currencyCode; provide a safe default (e.g., currencyCode ?: "$") or validate before use. Ensure all platform targets (Android/Desktop/Native) follow consistent null handling, and consider centralizing currencyCode normalization in the common layer if feasible. Add tests covering null currencyCode to prevent NPEs.
Applied to files:
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
📚 Learning: 2026-04-01T05:03:14.323Z
Learnt from: kartikey004
Repo: openMF/mifos-x-field-officer-app PR: 2659
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanDisbursement/LoanAccountDisbursementScreen.kt:190-195
Timestamp: 2026-04-01T05:03:14.323Z
Learning: In this repo, existing `SelectableDates` / `SelectableDates.isSelectableDate(utcTimeMillis: Long)` implementations use `Clock.System.now().toEpochMilliseconds()` (UTC epoch millis) for date-boundary checks. During PR reviews, do not flag these checks for not using a timezone-aware `LocalDate` start-of-day approach; treat it as an established project-wide pattern. If a change is desired, handle it as a coordinated project-wide improvement rather than as a per-PR review issue.
Applied to files:
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
🔇 Additional comments (7)
cmp-shared/cmp_shared.podspec (1)
53-53: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanRepaymentRepositoryImp.kt (1)
53-56: LGTM!feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt (5)
131-138: Layer 2 composable receivesNavController— architecture violation.This was flagged in a previous review.
LoanRepaymentContent(Layer 2) should only receivestateand a singleonActionlambda. ThenavControlleris passed solely forMifosBreadcrumbNavBarat line 176 and should be lifted to Layer 1.
81-104: LGTM!
106-129: LGTM!
293-361: LGTM!
363-438: LGTM!
|
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
2 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-#Issue_Number
Screen_recording_20260524_173046.webm