feat(loan): implement loan foreclosure screen#2680
Open
Kartikey15dem wants to merge 2 commits into
Open
Conversation
Summary by CodeRabbit
WalkthroughAdds a complete loan foreclosure feature: network DTOs and LoanService endpoints, DataManager methods, mappers and domain models, repository interface and implementation with DI binding, ViewModel orchestration and validation, Compose screen with constrained date picker and form, navigation route, and string resources. Loan Foreclosure Feature
Sequence DiagramsequenceDiagram
participant UI as Compose UI
participant VM as LoanForeclosureViewModel
participant Repo as LoanForeclosureRepositoryImp
participant DataMgr as DataManagerLoan
participant Service as LoanService
UI->>VM: user opens screen / picks date / taps submit
VM->>Repo: getLoanForeclosureTemplate(loanId, date, fmt, locale)
Repo->>DataMgr: getLoanForeclosureTemplate(...)
DataMgr->>Service: GET /loans/{id}/transactions?transactionDate=...
Service-->>DataMgr: HttpResponse (template JSON)
DataMgr-->>Repo: LoanForeclosureTemplateDto
Repo-->>VM: DataState.Success(template) or DataState.Error
VM->>UI: update ForeclosureState (amounts, enabled)
UI->>VM: Submit action
VM->>Repo: submitLoanForeclosure(loanId, input)
Repo->>DataMgr: submitLoanForeclosure(id, requestDto)
DataMgr->>Service: POST /loans/{id}/foreclosure (body)
Service-->>DataMgr: HttpResponse (status)
DataMgr-->>Repo: success or throw
Repo-->>VM: DataState<Unit>
VM->>UI: show overlay / finish or snackbar on error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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: 6
🤖 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
`@core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.kt`:
- Around line 19-25: The repository method getLoanForeclosureTemplate should be
a one-shot suspend function rather than a Flow; change the signature from fun
getLoanForeclosureTemplate(...): Flow<DataState<LoanForeclosureTemplate>> to
suspend fun getLoanForeclosureTemplate(...): DataState<LoanForeclosureTemplate>,
update all implementations of LoanForeclosureRepository to return a DataState
directly (remove Flow/emit usage and any Flow builders), adjust callers to call
the suspend function (await result instead of collecting a Flow), and remove
unused Flow imports; preserve parameter names (loanId, transactionDate,
dateFormat, locale) and ensure error/result wrapping uses DataState as before.
In
`@core/model/src/commonMain/kotlin/com/mifos/core/model/objects/template/loan/LoanForeclosureTemplate.kt`:
- Around line 16-34: Remove the network serialization annotations from the
domain models by deleting the `@Serializable` annotation (and its import) applied
to both LoanForeclosureTemplate and SubmitLoanForeclosureInput so these
core:model classes are not coupled to kotlinx.serialization; leave the data
classes intact (and keep/remove `@Parcelize` only if platform constraints require
it) and ensure no serialization-specific imports remain in the file.
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt`:
- Around line 418-438: The getLoanForeclosureTemplate function in
DataManagerLoan.kt currently returns a Flow but is a one-shot request; change
its signature to a suspend function that returns LoanForeclosureTemplateDto
(remove Flow), call mBaseApiManager.loanService.getLoanForeclosureTemplate(...)
directly in a suspend context, check response.status.isSuccess() and throw
IllegalStateException(extractErrorMessage(response)) on failure, then decode and
return the DTO with Json { ignoreUnknownKeys = true
}.decodeFromString<LoanForeclosureTemplateDto>(response.bodyAsText()) so the API
follows the one-shot suspend/raw-response contract.
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt`:
- Around line 201-207: The getLoanForeclosureTemplate function is modeled as a
Flow but per network guidelines it is a one-shot GET; change its signature to a
suspend function returning a raw HttpResponse (i.e., replace fun
getLoanForeclosureTemplate(...): Flow<HttpResponse> with suspend fun
getLoanForeclosureTemplate(...): HttpResponse), update any callers to await the
suspend call instead of collecting a Flow, and remove/adjust any Flow-related
imports/usages in LoanService.kt and related call sites so the endpoint conforms
to the one-shot API convention.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.kt`:
- Around line 170-174: The note field doesn't reflect validation state; update
the MifosOutlinedTextField (the one bound to state.note and onValueChange {
onAction(ForeclosureAction.OnNoteChange(it)) }) to pass the validation flag from
the ViewModel by adding isError = state.isNoteError so the field visually
indicates errors when the ViewModel sets the note validation flag.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.kt`:
- Around line 76-119: fetchTemplate currently mutates mutableStateFlow directly
inside the Flow collector; change it to map repository DataState results into
ForeclosureAction.Internal actions and dispatch them via sendAction(...) instead
of updating state there. Specifically, add a new ForeclosureAction.Internal
subtype for template results (success, loading, error), update fetchTemplate to
collect repository.getLoanForeclosureTemplate and call sendAction(...) with the
appropriate Internal action for DataState.Loading, DataState.Success (wrap
template DTO), and DataState.Error (include message/exception). Then implement
handling of that new Internal action inside handleAction(...) to perform the
actual mutableStateFlow.update and sendEvent(ForeclosureEvent.ShowSnackbar(...))
for IllegalStateException cases so all state mutations follow the reducer
pattern.
🪄 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: 032a0d67-7a2a-42ec-8151-3682c775b587
📒 Files selected for processing (16)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/template/loan/LoanForeclosureTemplate.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureSubmitRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.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/loanForeclosure/LoanForeclosureScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
**/{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/di/RepositoryModule.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureSubmitRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/template/loan/LoanForeclosureTemplate.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/di/RepositoryModule.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureSubmitRequestDto.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreenRoute.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/template/loan/LoanForeclosureTemplate.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.kt
**/network/**/*.kt
⚙️ CodeRabbit configuration file
**/network/**/*.kt: Network Layer API Guidelines:
- One-shot API calls MUST use
suspend, return raw responseT, and MUST NOT use Flow or DataState.- Streaming APIs MUST return
Flow<T>and MUST NOT usesuspendor DataState.
Files:
core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureSubmitRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.kt
**/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/LoanForeclosureRepository.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/LoanForeclosureRepositoryImp.kt
**/core/model/**/*.kt
⚙️ CodeRabbit configuration file
**/core/model/**/*.kt: Domain Model Rules (:core:model):
- Classes defined in this module are strictly Domain Models.
- They must be plain Kotlin data classes.
- They MUST NOT contain network-specific annotations (e.g.,
@Serializable) or database-specific annotations (e.g.,@Entity).- Rule: These are the ONLY models that should be passed to or used within ViewModels and UI Screens.
- Flag: If a Domain model is used directly as an API request/response body or a Room database table.
Files:
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/template/loan/LoanForeclosureTemplate.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
**/*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/loanForeclosure/LoanForeclosureViewModel.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/loanForeclosure/LoanForeclosureScreen.kt
🧠 Learnings (4)
📚 Learning: 2026-02-06T13:15:16.968Z
Learnt from: sahilshivekar
Repo: openMF/android-client PR: 2603
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt:43-106
Timestamp: 2026-02-06T13:15:16.968Z
Learning: Guideline: When a Kotlin function parameter is nullable (e.g., balance: Double?, currencyCode: String?, maximumFractionDigits: Int?) and downstream calls require a non-null value, add null-safety handling in all implementations. Specifically, avoid calling Currency.getInstance(currencyCode) with a possibly null currencyCode; provide a safe default (e.g., currencyCode ?: "$") or validate before use. Ensure all platform targets (Android/Desktop/Native) follow consistent null handling, and consider centralizing currencyCode normalization in the common layer if feasible. Add tests covering null currencyCode to prevent NPEs.
Applied to files:
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureSubmitRequestDto.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreenRoute.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/template/loan/LoanForeclosureTemplate.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.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/di/RepositoryModule.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureSubmitRequestDto.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreenRoute.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/template/loan/LoanForeclosureTemplate.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.kt
📚 Learning: 2026-03-16T11:58:32.496Z
Learnt from: sahilshivekar
Repo: openMF/mifos-x-field-officer-app PR: 2647
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt:96-96
Timestamp: 2026-03-16T11:58:32.496Z
Learning: In feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt (Kotlin/Compose Multiplatform), navigateToDashboard uses a direct method reference navController::navigateToLoanDashboardScreen without an ID guard. This is intentional: LoanAccountProfileScreen always provides a valid loanId, and LoanDashboardScreen handles invalid/missing data by showing a 'Failed to fetch Loan details' error state. Do not flag the absence of an ID guard here in future reviews.
Applied to files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
📚 Learning: 2025-12-31T08:19:18.178Z
Learnt from: amanna13
Repo: openMF/android-client PR: 2569
File: core-base/designsystem/src/commonMain/kotlin/template/core/base/designsystem/layout/KptResponsiveLayout.kt:140-153
Timestamp: 2025-12-31T08:19:18.178Z
Learning: When syncing template code from kmp-project-template in the android-client repo, avoid introducing functional or behavioral fixes in the synced template code in the initial sync PR. Defer such fixes to follow-up issues to keep the PR focused on template syncing only. If a fix is necessary, create a separate follow-up issue/PR and document the scope so maintainers understand that changes are deferred to template-related refinement.
Applied to files:
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/template/loan/LoanForeclosureTemplate.kt
🔇 Additional comments (15)
core/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.kt (1)
14-22: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureSubmitRequestDto.kt (1)
14-20: LGTM!core/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.kt (1)
48-48: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt (1)
440-451: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt (1)
209-213: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.kt (1)
17-33: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.kt (1)
26-29: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.kt (1)
28-65: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.kt (1)
49-49: LGTM!Also applies to: 119-119, 202-202
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.kt (1)
30-46: LGTM!Also applies to: 48-75, 121-163, 165-218
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.kt (1)
21-21: LGTM!Also applies to: 45-45
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreenRoute.kt (1)
17-37: LGTM!feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.kt (1)
60-109: LGTM!Also applies to: 111-169, 175-199
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt (1)
28-28: LGTM!Also applies to: 115-118
feature/loan/src/commonMain/composeResources/values/strings.xml (1)
479-487: LGTM!
There was a problem hiding this comment.
Pull request overview
Implements a new Loan Foreclosure flow end-to-end (UI screen + navigation + ViewModel state machine + repository + network wiring) within the loan feature.
Changes:
- Added
LoanForeclosureScreenUI, route, andLoanForeclosureViewModelwith template refresh + submit handling. - Introduced foreclosure repository + DI bindings, plus network service endpoints/DTOs/mappers and DataManager integration.
- Added foreclosure-related string resources and registered the screen in the loan navigation graph.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt | Registers the new foreclosure screen in the loan nav graph. |
| feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.kt | Adds foreclosure state/action/event model, template loading, and submit logic. |
| feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreenRoute.kt | Defines the typed navigation route and navigation helpers for foreclosure. |
| feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.kt | Implements the Compose UI (form, date picker, error/loading states, overlay loader). |
| feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.kt | Adds Koin ViewModel binding for LoanForeclosureViewModel. |
| feature/loan/src/commonMain/composeResources/values/strings.xml | Adds new localized strings for the foreclosure UI. |
| core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt | Adds API endpoints for foreclosure template fetch and submit. |
| core/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.kt | Adds DTO for foreclosure template response. |
| core/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureSubmitRequestDto.kt | Adds DTO for foreclosure submit request body. |
| core/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.kt | Adds DTO↔domain mapping for foreclosure template and submit input. |
| core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt | Adds DataManager methods to call foreclosure endpoints and decode responses. |
| core/model/src/commonMain/kotlin/com/mifos/core/model/objects/template/loan/LoanForeclosureTemplate.kt | Adds domain models for foreclosure template and submit payload. |
| core/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.kt | Adds a new endpoint constant. |
| core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.kt | Implements repository operations for foreclosure template + submit. |
| core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.kt | Defines repository interface for foreclosure operations. |
| core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.kt | Registers the foreclosure repository implementation with DI. |
Comments suppressed due to low confidence (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.kt:128
isNoteErroris set insubmit()when the note is blank, butisSubmitEnabledalready disables the submit button whennote.isBlank(), so this branch is effectively unreachable in normal UI flow. Either (a) removeisNoteErrorand the redundant check, or (b) allow the submit attempt and surface the validation error in the UI (seeLoanForeclosureScreennote field).
private fun submit() {
if (state.note.isBlank()) {
mutableStateFlow.update { it.copy(isNoteError = true) }
return
}
mutableStateFlow.update { it.copy(isOverLayLoadingActive = true) }
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.kt:174
- The note field doesn’t consume
state.isNoteError, so users won’t see any inline validation feedback even though the ViewModel tracks it. Consider passing anerrormessage intoMifosOutlinedTextFieldwhenisNoteErroris true (and add a dedicated string resource for the required-note message).
MifosOutlinedTextField(
value = state.note,
onValueChange = { onAction(ForeclosureAction.OnNoteChange(it)) },
label = stringResource(Res.string.feature_loan_note),
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Kartikey15dem
force-pushed
the
active-loan-foreclosure-screen
branch
from
b5c7c42 to
3528b46
Compare
biplab1
changed the title
biplab1
requested changes
May 13, 2026
biplab1
left a comment
biplab1
left a comment
Contributor
There was a problem hiding this comment.
Please address the following requested changes.
Contributor
|
Please put the domain models inside: |
Kartikey15dem
force-pushed
the
active-loan-foreclosure-screen
branch
from
3528b46 to
22bbe34
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
feature/loan/src/commonMain/composeResources/values/strings.xml (1)
481-481: 💤 Low valueConsider reusing an existing key to avoid duplication.
The UI text "Interest" already appears in two existing keys:
feature_loan_table_header_interest(line 212) andfeature_loan_interest_short(line 361). If the context allows, consider reusing one of these instead of introducingfeature_loan_interest. As per coding guidelines, the guideline advises to avoid multiple keys representing the same UI text.🤖 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/composeResources/values/strings.xml` at line 481, You added a duplicate string key feature_loan_interest; instead of introducing it, reuse an existing key (prefer feature_loan_interest_short if the UI uses the short label, or feature_loan_table_header_interest if it’s the table header) — replace usages of feature_loan_interest with the appropriate existing key in the layout/code and remove the new feature_loan_interest entry from strings.xml so the same UI text isn’t represented by multiple keys.
🤖 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
`@core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.kt`:
- Around line 22-25: The repository performs a network call directly in
LoanForeclosureRepositoryImp using dataManager without the same IO dispatch and
network-availability guard used elsewhere; update the template-fetch path in
LoanForeclosureRepositoryImp to run on the IO dispatcher from DispatcherManager
and wrap the dataManager network call (e.g., the template fetch function) with
the project’s network-availability check and try/catch error handling, returning
the same failure/result type as the other path so behavior is consistent with
other repository implementations.
- Around line 42-43: In LoanForeclosureRepositoryImp, preserve coroutine
cancellation by adding an explicit catch for CancellationException that rethrows
it before the existing generic Exception handlers; specifically, in the two
places where you currently have "catch (e: Exception) { DataState.Error(e) }",
insert "catch (ce: CancellationException) { throw ce }" immediately before those
generic catches so CancellationException is not swallowed by the repository
methods in LoanForeclosureRepositoryImp.
---
Nitpick comments:
In `@feature/loan/src/commonMain/composeResources/values/strings.xml`:
- Line 481: You added a duplicate string key feature_loan_interest; instead of
introducing it, reuse an existing key (prefer feature_loan_interest_short if the
UI uses the short label, or feature_loan_table_header_interest if it’s the table
header) — replace usages of feature_loan_interest with the appropriate existing
key in the layout/code and remove the new feature_loan_interest entry from
strings.xml so the same UI text isn’t represented by multiple keys.
🪄 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: 41a849c9-b3ff-4e45-b6f7-5932913a4b27
📒 Files selected for processing (16)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureInput.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureTemplate.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.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/loanForeclosure/LoanForeclosureScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
✅ Files skipped from review due to trivial changes (1)
- core/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreenRoute.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.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). (6)
- GitHub Check: PR Checks / Build Android Application
- GitHub Check: PR Checks / Build Desktop Application (macos-latest)
- GitHub Check: PR Checks / Build Web Application
- GitHub Check: PR Checks / Build iOS App
- GitHub Check: PR Checks / Build Desktop Application (ubuntu-latest)
- GitHub Check: PR Checks / Build Desktop Application (windows-latest)
🧰 Additional context used
📓 Path-based instructions (7)
**/network/**/*.kt
⚙️ CodeRabbit configuration file
**/network/**/*.kt: Network Layer API Guidelines:
- One-shot API calls MUST use
suspend, return raw responseT, and MUST NOT use Flow or DataState.- Streaming APIs MUST return
Flow<T>and MUST NOT usesuspendor DataState.
Files:
core/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.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/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureTemplate.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureInput.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.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/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureTemplate.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureInput.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt
**/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/LoanForeclosureRepository.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/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/LoanForeclosureRepositoryImp.kt
**/core/model/**/*.kt
⚙️ CodeRabbit configuration file
**/core/model/**/*.kt: Domain Model Rules (:core:model):
- Classes defined in this module are strictly Domain Models.
- They must be plain Kotlin data classes.
- They MUST NOT contain network-specific annotations (e.g.,
@Serializable) or database-specific annotations (e.g.,@Entity).- Rule: These are the ONLY models that should be passed to or used within ViewModels and UI Screens.
- Flag: If a Domain model is used directly as an API request/response body or a Room database table.
Files:
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureTemplate.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureInput.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/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureTemplate.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureInput.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.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/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureTemplate.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureInput.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt
🔇 Additional comments (8)
feature/loan/src/commonMain/composeResources/values/strings.xml (1)
479-487: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureRequestDto.kt (1)
14-20: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt (1)
27-27: LGTM!Also applies to: 201-213
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt (1)
27-29: LGTM!Also applies to: 418-449
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureTemplate.kt (1)
12-19: LGTM!core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureInput.kt (1)
12-17: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.kt (1)
17-33: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.kt (1)
16-29: LGTM!
feat(feature/loan): loan foreclosurescreen feat(feature/loan): loan foreclosurescreen feat(feature/loan): loan foreclosurescreen feat(feature/loan): loan foreclosurescreen feat(feature/loan): loan foreclosurescreen
Kartikey15dem
force-pushed
the
active-loan-foreclosure-screen
branch
from
22bbe34 to
b56ac00
Compare
biplab1
reviewed
May 15, 2026
Contributor
There was a problem hiding this comment.
Please refer the implementation here: https://github.com/openMF/mifos-x-field-officer-app/pull/2666/changes#diff-de1e2324a12af2f1bc196e6fb5e2b1ddce433f1eaf890f106292808b88143568 and update this class accordingly.
Contributor
|
Also create Room entities corresponding to the DTOs used to consume API response like template and place them in model, add mappers too. Please carefully study the changes here https://github.com/openMF/mifos-x-field-officer-app/pull/2666/changes#top and align everything properly. |
Contributor
Author
|
|
There was a problem hiding this comment.
♻️ Duplicate comments (3)
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.kt (2)
42-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve coroutine cancellation by rethrowing
CancellationException.Both generic catches currently absorb coroutine cancellation and convert it into
DataState.Error, which can keep cancelled jobs alive unexpectedly.Suggested minimal fix
+import kotlinx.coroutines.CancellationException import kotlinx.coroutines.withContext @@ - } catch (e: Exception) { + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { DataState.Error(e) } @@ - } catch (e: Exception) { + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { DataState.Error(e) }Also applies to: 56-57
🤖 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/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.kt` around lines 42 - 43, The catch blocks in LoanForeclosureRepositoryImp (class LoanForeclosureRepositoryImp) are currently catching Exception and converting cancellations into DataState.Error; change the handlers to preserve coroutine cancellation by either adding a specific catch for CancellationException and rethrowing it, or by checking inside the existing catch (e: Exception) and rethrowing if e is a CancellationException, then continue to return DataState.Error for other exceptions (apply to both catch sites currently returning DataState.Error).
22-25:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd repository-level network availability guard for foreclosure calls.
This implementation currently depends only on exception handling for connectivity failures; it does not apply an explicit network-availability check pattern before performing network work.
Suggested direction
Inject and apply the same repository network-check wrapper used in other data implementations for both
getLoanForeclosureTemplateandsubmitLoanForeclosure, then keep the existingDataStatemapping behavior on top.As per coding guidelines
**/data/repositoryImp/**/*.kt: “MUST perform network availability checks, handle exceptions, and apply appropriate dispatchers.”Also applies to: 33-58
🤖 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/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.kt` around lines 22 - 25, The LoanForeclosureRepositoryImp currently lacks an explicit network-availability guard; update the class constructor to inject the repository network-check wrapper used by other data implementations (e.g., withNetworkAvailabilityCheck / networkAvailabilityGuard), and wrap the network operations inside getLoanForeclosureTemplate and submitLoanForeclosure with that guard before calling DataManagerLoan, preserving the existing DataState mapping and exception handling and ensuring the calls still run on DispatcherManager-provided dispatchers.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.kt (1)
170-174:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExpose note validation error state in the note input.
At Line 170 in LoanForeclosureScreen.kt,
state.isNoteErroris never bound toMifosOutlinedTextField, so required-note validation has no inline visual feedback.🤖 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/loanForeclosure/LoanForeclosureScreen.kt` around lines 170 - 174, The note input in LoanForeclosureScreen is not showing validation state because state.isNoteError is not passed into MifosOutlinedTextField; update the MifosOutlinedTextField invocation (in LoanForeclosureScreen.kt) to bind the error state and message by supplying the isError flag (e.g., isError = state.isNoteError) and an appropriate errorText/placeholder (e.g., when state.isNoteError then stringResource(Res.string.feature_loan_note_required) or similar) alongside the existing value, onValueChange (ForeclosureAction.OnNoteChange) and label so the field shows inline validation feedback.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.kt (1)
129-149: ⚡ Quick winExtract the date picker dialog into a dedicated composable.
In LoanForeclosureScreen.kt, keeping the dialog inline inside
ForeclosureFormContentmakes this screen harder to maintain and conflicts with the screen UI-structure rule.As per coding guidelines: "Dialogs must be separated into their own composables."
🤖 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/loanForeclosure/LoanForeclosureScreen.kt` around lines 129 - 149, Extract the inline DatePickerDialog into a new composable (e.g., TransactionDatePickerDialog) and call it from the existing location when state.showTransactionDatePick is true; the new composable should accept the DatePickerState (datePickerState), a visible flag, and callbacks for onDismiss/onCancel (invoke ForeclosureAction.OnTransactionDatePick(false)) and onConfirm (if datePickerState.selectedDateMillis != null invoke ForeclosureAction.OnTransactionDateSelected(millis) else invoke OnTransactionDatePick(false)); inside the composable render DatePicker(state = datePickerState) and reuse the same confirm/cancel TextButton labels (Res.string.feature_loan_select and Res.string.feature_loan_cancel) so behavior and strings remain unchanged.
🤖 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.
Duplicate comments:
In
`@core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.kt`:
- Around line 42-43: The catch blocks in LoanForeclosureRepositoryImp (class
LoanForeclosureRepositoryImp) are currently catching Exception and converting
cancellations into DataState.Error; change the handlers to preserve coroutine
cancellation by either adding a specific catch for CancellationException and
rethrowing it, or by checking inside the existing catch (e: Exception) and
rethrowing if e is a CancellationException, then continue to return
DataState.Error for other exceptions (apply to both catch sites currently
returning DataState.Error).
- Around line 22-25: The LoanForeclosureRepositoryImp currently lacks an
explicit network-availability guard; update the class constructor to inject the
repository network-check wrapper used by other data implementations (e.g.,
withNetworkAvailabilityCheck / networkAvailabilityGuard), and wrap the network
operations inside getLoanForeclosureTemplate and submitLoanForeclosure with that
guard before calling DataManagerLoan, preserving the existing DataState mapping
and exception handling and ensuring the calls still run on
DispatcherManager-provided dispatchers.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.kt`:
- Around line 170-174: The note input in LoanForeclosureScreen is not showing
validation state because state.isNoteError is not passed into
MifosOutlinedTextField; update the MifosOutlinedTextField invocation (in
LoanForeclosureScreen.kt) to bind the error state and message by supplying the
isError flag (e.g., isError = state.isNoteError) and an appropriate
errorText/placeholder (e.g., when state.isNoteError then
stringResource(Res.string.feature_loan_note_required) or similar) alongside the
existing value, onValueChange (ForeclosureAction.OnNoteChange) and label so the
field shows inline validation feedback.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.kt`:
- Around line 129-149: Extract the inline DatePickerDialog into a new composable
(e.g., TransactionDatePickerDialog) and call it from the existing location when
state.showTransactionDatePick is true; the new composable should accept the
DatePickerState (datePickerState), a visible flag, and callbacks for
onDismiss/onCancel (invoke ForeclosureAction.OnTransactionDatePick(false)) and
onConfirm (if datePickerState.selectedDateMillis != null invoke
ForeclosureAction.OnTransactionDateSelected(millis) else invoke
OnTransactionDatePick(false)); inside the composable render DatePicker(state =
datePickerState) and reuse the same confirm/cancel TextButton labels
(Res.string.feature_loan_select and Res.string.feature_loan_cancel) so behavior
and strings remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a081e227-25b7-43fc-8258-88b8a3c9d63e
📒 Files selected for processing (16)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureInput.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureTemplate.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.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/loanForeclosure/LoanForeclosureScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
✅ Files skipped from review due to trivial changes (1)
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureInput.kt
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
**/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
**/core/model/**/*.kt
⚙️ CodeRabbit configuration file
**/core/model/**/*.kt: Domain Model Rules (:core:model):
- Classes defined in this module are strictly Domain Models.
- They must be plain Kotlin data classes.
- They MUST NOT contain network-specific annotations (e.g.,
@Serializable) or database-specific annotations (e.g.,@Entity).- Rule: These are the ONLY models that should be passed to or used within ViewModels and UI Screens.
- Flag: If a Domain model is used directly as an API request/response body or a Room database table.
Files:
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureTemplate.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/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureTemplate.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.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/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureTemplate.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreenRoute.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureRequestDto.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt
**/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/LoanForeclosureRepository.kt
**/network/**/*.kt
⚙️ CodeRabbit configuration file
**/network/**/*.kt: Network Layer API Guidelines:
- One-shot API calls MUST use
suspend, return raw responseT, and MUST NOT use Flow or DataState.- Streaming APIs MUST return
Flow<T>and MUST NOT usesuspendor DataState.
Files:
core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.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/loanForeclosure/LoanForeclosureScreen.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/loanForeclosure/LoanForeclosureViewModel.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/LoanForeclosureRepositoryImp.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:
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureTemplate.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreenRoute.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureRequestDto.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.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/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureTemplate.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreenRoute.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureRequestDto.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanForeclosureRepositoryImp.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.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 (13)
core/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureTemplateDto.kt (1)
14-22: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/model/loan/LoanForeclosureRequestDto.kt (1)
14-20: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt (1)
27-27: LGTM!Also applies to: 201-213
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt (1)
418-449: LGTM!core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/foreclosure/LoanForeclosureTemplate.kt (1)
12-19: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/mappers/loan/LoanForeclosureMapper.kt (1)
17-33: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanForeclosureRepository.kt (1)
18-28: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.kt (1)
49-50: LGTM!Also applies to: 119-120, 202-202
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureViewModel.kt (1)
31-79: LGTM!Also applies to: 81-117, 120-173, 175-231
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanForeclosure/LoanForeclosureScreenRoute.kt (1)
17-24: LGTM!Also applies to: 26-37
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.kt (1)
21-21: LGTM!Also applies to: 45-45
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt (1)
28-28: LGTM!Also applies to: 45-45, 81-81, 117-120
feature/loan/src/commonMain/composeResources/values/strings.xml (1)
479-486: 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
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-#687
Summary
Implemented the Loan Foreclosure flow.
UI
foreclosure.webm
Key behavior
Template loading / refresh
Loading → Success/Errorstate model.IllegalStateException, e.g. invalid transaction date) are treated as non-fatal:Submit handling
isOverLayLoadingActive) so the form stays visible but interaction is blocked while the request is in-flight.IllegalStateException→ Snackbar (non-fatal)