feat(loan): implement loan charge-off screen#2696
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
Summary by CodeRabbit
WalkthroughAdds a loan charge-off flow with new models, network API wiring, repository and domain layers, a charge-off screen and view model, and navigation entry points integrated into loan actions. ChangesLoan Charge Off Feature Layer-by-layer breakdown
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)Charge-off submission flowsequenceDiagram
participant User
participant LoanChargeOffScreen
participant LoanChargeOffViewModel
participant LoanChargeOffUseCase
participant LoanChargeOffRepository
participant DataManagerLoan
User->>LoanChargeOffScreen: opens screen with loanId
LoanChargeOffScreen->>LoanChargeOffViewModel: init and loadTemplate()
LoanChargeOffViewModel->>LoanChargeOffUseCase: getChargeOffTemplate(loanId)
LoanChargeOffUseCase->>LoanChargeOffRepository: getChargeOffTemplate(loanId)
LoanChargeOffRepository->>DataManagerLoan: getChargeOffTemplate(loanId)
DataManagerLoan-->>LoanChargeOffRepository: template DTO
LoanChargeOffRepository-->>LoanChargeOffViewModel: DataState<List<ChargeOffReasonOption>>
User->>LoanChargeOffScreen: selects reason and submits
LoanChargeOffScreen->>LoanChargeOffViewModel: Submit
LoanChargeOffViewModel->>LoanChargeOffUseCase: chargeOff(loanId, LoanChargeOffInput)
LoanChargeOffUseCase->>LoanChargeOffRepository: chargeOff(loanId, LoanChargeOffInput)
LoanChargeOffRepository->>DataManagerLoan: chargeOff(loanId, requestDto)
DataManagerLoan-->>LoanChargeOffRepository: response DTO
LoanChargeOffRepository-->>LoanChargeOffViewModel: DataState<Unit>
LoanChargeOffViewModel-->>LoanChargeOffScreen: dialog / success event
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 |
sahilshivekar
force-pushed
the
charge-off-screen
branch
4 times, most recently
from
16c4458 to
da0d6ac
Compare
revanthkumarJ
previously approved these changes
Jul 1, 2026
Contributor
|
@sahilshivekar upload video or screenshots |
Contributor
Author
@revanthkumarJ The app isn't working now. Server is responding with status code 502 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionItem.kt (1)
391-395: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWire the visible
ChargeOffaction to navigationLoanAccountActionViewModel.kt still emits
LoanAccountActionItem.ChargeOff, but LoanAccountActionNavigation.kt maps that case to{}while onlyLoanChargeOffis routed. Tapping the charge-off row does nothing.🤖 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/loanAccountAction/LoanAccountActionItem.kt` around lines 391 - 395, The visible ChargeOff action is emitted by LoanAccountActionViewModel but has no navigation target, so tapping it is a no-op. Update LoanAccountActionNavigation to handle LoanAccountActionItem.ChargeOff the same way the existing LoanChargeOff route is handled, and ensure the ChargeOff action in LoanAccountActionItem is wired to the actual charge-off destination instead of an empty block.
🧹 Nitpick comments (3)
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/LoanChargeOffInput.kt (1)
14-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the shared short-date constant
LoanChargeOffInput.kt hardcodes
"dd-MM-yyyy", while DateHelper.kt already defines the same pattern asSHORT_MONTH. Reuse that constant, or add one alongsideDateConstants.DATE_FORMAT, so the request stays aligned with the rest of the date-format definitions.🤖 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/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/LoanChargeOffInput.kt` around lines 14 - 21, LoanChargeOffInput currently hardcodes the short date pattern, so update the LoanChargeOffInput data class to reuse the shared date-format constant instead of repeating the literal. Use the existing DateHelper/DateConstants symbols already in the codebase, such as SHORT_MONTH or a shared constant alongside DateConstants.DATE_FORMAT, and replace the default dateFormat value in LoanChargeOffInput with that shared reference.core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt (1)
118-128: 📐 Maintainability & Code Quality | 🔵 TrivialHeads up: this is a
coremodule — extra scrutiny time. 🎩These new
chargeOff/getChargeOffTemplatemethods land inDataManagerLoan.kt, a shared foundationalcoremodule consumed by every downstream feature. Nothing wrong with the code itself, just flagging per path instructions thatcore-module changes deserve careful review attention since ripple effects are wide-reaching (this note applies to the whole cohort undercore/, not just this file).As per path instructions, "Changes in
core-basemodule must be treated as high-impact... Clearly highlight thatcore-baseis a shared foundational module and requires extra review attention."🤖 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/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt` around lines 118 - 128, The new getChargeOffTemplate and chargeOff additions in DataManagerLoan are in a shared core module and should be treated as high-impact changes. Update the surrounding module/documentation or PR notes to explicitly call out that DataManagerLoan and its new loanService passthroughs affect downstream consumers across the app, so they require extra review attention and regression checking.Source: Path instructions
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.kt (1)
167-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
DatePickerDialoginto its own composable.The date-picker dialog is embedded inline inside
LoanChargeOffForm, which also renders the dropdown, text fields, and submit button. Per UI structure guidelines, dialogs should be separated into their own composables rather than embedded inline in a larger screen/form composable.As per path instructions, "Dialogs must be separated into their own composables" and "Do not embed dialogs inline within complex main screens."
🤖 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/loanChargeOff/LoanChargeOffScreen.kt` around lines 167 - 188, The DatePickerDialog is embedded inline inside LoanChargeOffForm, which violates the dialog separation guideline. Extract the entire date-picker block into its own composable (for example, a dedicated LoanChargeOffDatePickerDialog) and have LoanChargeOffForm call it when state.showDatePicker is true. Keep the dialog’s current behavior by moving the onDismissRequest, confirmButton, dismissButton, and DatePicker state handling into the new composable, while preserving the existing LoanChargeOffAction callbacks.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.kt`:
- Around line 228-232: The LoanChargeOff screen is formatting the transaction
date directly in the Composable, which violates the UI-layer formatting rule.
Move the `DateHelper.getDateAsStringFromLong` logic out of `LoanChargeOffScreen`
and into the ViewModel by adding a preformatted `transactionDateText` (or
similar) field on `LoanChargeOffState`, then update it whenever
`transactionDate` changes and bind `MifosDatePickerTextField` to that state
value instead.
- Around line 90-96: LoanChargeOffScreenContent currently violates the Screen
architecture by taking a NavController in addition to state and onAction. Move
the MifosBreadcrumbNavBar(navController) usage out of LoanChargeOffScreenContent
into LoanChargeOffScreen, or replace the navController dependency with a
narrowed composable slot/lambda so the Layer 2 composable only accepts state and
a single onAction callback. Keep the navigation reference confined to the entry
composable and update LoanChargeOffScreenContent’s signature accordingly.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreenRoute.kt`:
- Around line 24-29: `LoanChargeOffScreen` should not depend on `NavController`;
keep that dependency in the route and use it only where `MifosBreadcrumbNavBar`
needs it. Update `LoanChargeOffScreenRoute` so the
`composable<LoanChargeOffRoute>` block still creates/passes navigation behavior,
but `LoanChargeOffScreen` receives only `state` and `onAction` (plus any other
UI-only params). Adjust the `LoanChargeOffScreen` signature and its callers
accordingly, and keep `navController` usage confined to the route-level
breadcrumb setup.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffViewModel.kt`:
- Around line 42-68: The async start-state updates in LoanChargeOffViewModel
bypass the reducer by calling mutableStateFlow.update directly inside
loadTemplate() and submitChargeOff(). Replace those direct mutations with new
Internal start actions (for example, TemplateLoadStarted and
ChargeOffSubmissionStarted) sent via sendAction(...), and handle them in
handleAction()/reducer alongside ReceiveTemplateResult and
ReceiveChargeOffResult so all state changes flow through one path.
---
Outside diff comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionItem.kt`:
- Around line 391-395: The visible ChargeOff action is emitted by
LoanAccountActionViewModel but has no navigation target, so tapping it is a
no-op. Update LoanAccountActionNavigation to handle
LoanAccountActionItem.ChargeOff the same way the existing LoanChargeOff route is
handled, and ensure the ChargeOff action in LoanAccountActionItem is wired to
the actual charge-off destination instead of an empty block.
---
Nitpick comments:
In
`@core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/LoanChargeOffInput.kt`:
- Around line 14-21: LoanChargeOffInput currently hardcodes the short date
pattern, so update the LoanChargeOffInput data class to reuse the shared
date-format constant instead of repeating the literal. Use the existing
DateHelper/DateConstants symbols already in the codebase, such as SHORT_MONTH or
a shared constant alongside DateConstants.DATE_FORMAT, and replace the default
dateFormat value in LoanChargeOffInput with that shared reference.
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt`:
- Around line 118-128: The new getChargeOffTemplate and chargeOff additions in
DataManagerLoan are in a shared core module and should be treated as high-impact
changes. Update the surrounding module/documentation or PR notes to explicitly
call out that DataManagerLoan and its new loanService passthroughs affect
downstream consumers across the app, so they require extra review attention and
regression checking.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.kt`:
- Around line 167-188: The DatePickerDialog is embedded inline inside
LoanChargeOffForm, which violates the dialog separation guideline. Extract the
entire date-picker block into its own composable (for example, a dedicated
LoanChargeOffDatePickerDialog) and have LoanChargeOffForm call it when
state.showDatePicker is true. Keep the dialog’s current behavior by moving the
onDismissRequest, confirmButton, dismissButton, and DatePicker state handling
into the new composable, while preserving the existing LoanChargeOffAction
callbacks.
🪄 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: 26f9ea92-0ff1-4d13-b884-ccfa706bf0a6
📒 Files selected for processing (28)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/ChargeOffReasonOptionMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/LoanChargeOffInputMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanChargeOffRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanChargeOffRepositoryImpl.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/di/UseCaseModule.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/GetLoanChargeOffTemplateUseCase.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/LoanChargeOffUseCase.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/ChargeOffReasonOption.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/LoanChargeOffInput.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffResponseDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffTemplateDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/ui/src/commonMain/composeResources/drawable/charge_off.xmlfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionItem.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffState.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: PR Checks / Build Desktop Application (macos-latest)
- GitHub Check: PR Checks / Build Desktop Application (windows-latest)
- GitHub Check: PR Checks / Build iOS App
- GitHub Check: PR Checks / Build Web Application
- GitHub Check: PR Checks / Build Android Application
🧰 Additional context used
📓 Path-based instructions (11)
**/*.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/di/LoanModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionNavigation.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/LoanChargeOffInputMapper.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/di/UseCaseModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffState.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/LoanChargeOffUseCase.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffRequestDto.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/ChargeOffReasonOption.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/GetLoanChargeOffTemplateUseCase.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffResponseDto.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/LoanChargeOffInput.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/ChargeOffReasonOptionMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanChargeOffRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffTemplateDto.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionItem.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanChargeOffRepositoryImpl.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffViewModel.kt
**/data/mappers/**/*.kt
⚙️ CodeRabbit configuration file
**/data/mappers/**/*.kt: Mapper Consistency Rules (:core:data:mappers):
- All mapping logic MUST be centralized in this directory.
- Allowed mappings:
- DTO -> Domain Model
- Entity <-> Domain Model
- Flag: Any mapper functions found in any module outside of
:core:data:mappers.
Files:
core/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/LoanChargeOffInputMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/ChargeOffReasonOptionMapper.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/mappers/loan/LoanChargeOffInputMapper.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/di/UseCaseModule.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/LoanChargeOffUseCase.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffRequestDto.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/ChargeOffReasonOption.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/GetLoanChargeOffTemplateUseCase.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffResponseDto.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/LoanChargeOffInput.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/ChargeOffReasonOptionMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanChargeOffRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffTemplateDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanChargeOffRepositoryImpl.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.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/dto/loans/LoanChargeOffRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffResponseDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffTemplateDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt
**/network/dto/**/*.kt
⚙️ CodeRabbit configuration file
**/network/dto/**/*.kt: DTO Rules (:core:network:dto):
- Classes here are Data Transfer Objects (DTOs) used strictly for API communication.
- Rule: DTOs MUST NOT be used in ViewModels, Screens, or the Domain layer.
- Flag: If a DTO is returned directly by a Repository interface or used in UI state.
Files:
core/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffResponseDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffTemplateDto.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/ChargeOffReasonOption.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/LoanChargeOffInput.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/LoanChargeOffRepository.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/loanChargeOff/LoanChargeOffScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.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/LoanChargeOffRepositoryImpl.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/loanChargeOff/LoanChargeOffViewModel.kt
🧠 Learnings (4)
📚 Learning: 2026-02-06T13:15:16.968Z
Learnt from: sahilshivekar
Repo: openMF/android-client PR: 2603
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt:43-106
Timestamp: 2026-02-06T13:15:16.968Z
Learning: Guideline: When a Kotlin function parameter is nullable (e.g., balance: Double?, currencyCode: String?, maximumFractionDigits: Int?) and downstream calls require a non-null value, add null-safety handling in all implementations. Specifically, avoid calling Currency.getInstance(currencyCode) with a possibly null currencyCode; provide a safe default (e.g., currencyCode ?: "$") or validate before use. Ensure all platform targets (Android/Desktop/Native) follow consistent null handling, and consider centralizing currencyCode normalization in the common layer if feasible. Add tests covering null currencyCode to prevent NPEs.
Applied to files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionNavigation.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/LoanChargeOffInputMapper.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/di/UseCaseModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffState.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/LoanChargeOffUseCase.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffRequestDto.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/ChargeOffReasonOption.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/GetLoanChargeOffTemplateUseCase.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffResponseDto.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/LoanChargeOffInput.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/ChargeOffReasonOptionMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanChargeOffRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffTemplateDto.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionItem.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanChargeOffRepositoryImpl.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffViewModel.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/di/LoanModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionNavigation.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/LoanChargeOffInputMapper.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/di/UseCaseModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffState.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/LoanChargeOffUseCase.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffRequestDto.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/ChargeOffReasonOption.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/GetLoanChargeOffTemplateUseCase.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffResponseDto.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/LoanChargeOffInput.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/ChargeOffReasonOptionMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanChargeOffRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffTemplateDto.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionItem.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanChargeOffRepositoryImpl.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffViewModel.kt
📚 Learning: 2026-03-16T11:58:32.496Z
Learnt from: sahilshivekar
Repo: openMF/mifos-x-field-officer-app PR: 2647
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt:96-96
Timestamp: 2026-03-16T11:58:32.496Z
Learning: In feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt (Kotlin/Compose Multiplatform), navigateToDashboard uses a direct method reference navController::navigateToLoanDashboardScreen without an ID guard. This is intentional: LoanAccountProfileScreen always provides a valid loanId, and LoanDashboardScreen handles invalid/missing data by showing a 'Failed to fetch Loan details' error state. Do not flag the absence of an ID guard here in future reviews.
Applied to files:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
📚 Learning: 2026-02-16T08:26:18.398Z
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt:328-337
Timestamp: 2026-02-16T08:26:18.398Z
Learning: In core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt, ensure size tokens in AppSizes use raw dp-based names like dp5, dp18, dp20, dp42, dp48, dp72, dp100, dp120, dp128, etc., rather than relying solely on semantic names. This naming convention should be accepted for design token definitions in this file and should guide future token definitions without requiring semantic aliases.
Applied to files:
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.kt
🔇 Additional comments (27)
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/ChargeOffReasonOption.kt (1)
12-19: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffRequestDto.kt (1)
14-22: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffResponseDto.kt (1)
14-29: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffTemplateDto.kt (1)
14-27: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt (1)
26-28: LGTM!Also applies to: 79-89
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.kt (1)
48-48: LGTM!Also applies to: 118-118, 202-202
core/domain/src/commonMain/kotlin/com/mifos/core/domain/di/UseCaseModule.kt (1)
92-93: LGTM!Also applies to: 178-179
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt (2)
26-28: LGTM!Also applies to: 118-128
389-391: LGTM!Also applies to: 409-411
core/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/ChargeOffReasonOptionMapper.kt (1)
15-22: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/LoanChargeOffInputMapper.kt (1)
15-22: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanChargeOffRepository.kt (1)
16-21: LGTM!Both methods are
suspend funreturningDataState<T>with noFlowin sight — right in line withLoanChargeOffRepository.kt's one-shot repository contract. As per path instructions, "One-shot operations MUST usesuspend funand returnDataState<T>. MUST NOT use Flow."Source: Path instructions
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanChargeOffRepositoryImpl.kt (1)
28-47: LGTM!Nice boundary translation here —
LoanChargeOffRepositoryImpl.ktwraps calls inrunAsDataStatewithnetworkMonitor/dispatcher.io, and maps DTOs to domain models (toModel()) before returning — right in line with "Repositories act as the boundary translator. They MUST use mappers to convert DTOs... into Domain Models before returning data." Confirmed via Kotlin's spec-defined Unit-coercion that thechargeOffblock returning a discardedLoanChargeOffResponseDtoagainst a declaredDataState<Unit>compiles cleanly — no type mismatch here.Source: Path instructions
core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/GetLoanChargeOffTemplateUseCase.kt (1)
16-22: LGTM!core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/LoanChargeOffUseCase.kt (1)
16-23: LGTM!feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffState.kt (1)
17-35: LGTM!feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffViewModel.kt (1)
70-163: LGTM!feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.kt (1)
68-88: LGTM!core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.kt (1)
50-50: LGTM!Note: this touches a shared
coremodule file; as per path instructions, changes under**/{core-base,core}/**/*.ktshould get extra scrutiny, but this is a minimal, additive icon mapping with no behavior change elsewhere.Also applies to: 269-269
Source: Path instructions
core/ui/src/commonMain/composeResources/drawable/charge_off.xml (1)
15-16: 🩺 Stability & Availability | ⚡ Quick win
android:tint="?attr/colorControlNormal"is unsupported in Compose Multiplatform resources.This drawable lives under
commonMain/composeResources/drawable, so it's compiled for all KMP targets, not just Android. Official Kotlin Multiplatform docs instruct to "Remove any other Android-specific attributes for color adjustments like android:tint." when adding vector icons to multiplatform resources, since theme-attribute references (?attr/...) have no equivalent outside the Android theme system.🔧 Suggested fix
<vector xmlns:android="http://schemas.android.com/apk/res/android" android:width="24dp" android:height="24dp" android:viewportWidth="960" - android:viewportHeight="960" - android:tint="?attr/colorControlNormal"> + android:viewportHeight="960"> <path android:fillColor="`#e3e3e3`" android:pathData="..."/> </vector>Apply tinting at the Compose call site (e.g., via
Icon'stintparameter orColorFilter) instead of the XML attribute.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.kt (1)
39-42: LGTM!feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt (1)
78-95: LGTM!feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionNavigation.kt (1)
23-27: LGTM on the wiring itself — flagged the underlying duplicate-action-item concern separately in LoanAccountActionItem.kt.Also applies to: 82-82
feature/loan/src/commonMain/composeResources/values/strings.xml (1)
167-182: LGTM!feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreenRoute.kt (1)
17-18: LGTM!feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.kt (1)
22-22: LGTM!Also applies to: 39-39
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt (1)
31-32: LGTM!Also applies to: 90-93, 125-125
Contributor
ask victor once in slack and let me know once its working and screenshots or video is attached with this pr |
Contributor
Author
@revanthkumarJ I did that already, he isn't active |
sahilshivekar
force-pushed
the
charge-off-screen
branch
from
487bb4f to
c25c091
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.kt (1)
150-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
DatePickerDialoginto its own composable.The date-picker dialog is embedded inline inside
LoanChargeOffForm. Per path instructions, dialogs must be separated into their own composables rather than embedded inline within complex screens (theLoanChargeOffDialogat Line 238 already follows this pattern correctly).♻️ Suggested extraction
+@Composable +private fun LoanChargeOffDatePickerDialog( + dueDatePickerState: DatePickerState, + onConfirm: (Long) -> Unit, + onDismiss: () -> Unit, +) { + DatePickerDialog( + onDismissRequest = onDismiss, + confirmButton = { + TextButton( + onClick = { + dueDatePickerState.selectedDateMillis?.let(onConfirm) + onDismiss() + }, + ) { Text(stringResource(Res.string.ok)) } + }, + dismissButton = { + TextButton(onClick = onDismiss) { Text(stringResource(Res.string.cancel)) } + }, + ) { + DatePicker(state = dueDatePickerState) + } +}Then call it conditionally from
LoanChargeOffFormwhenstate.showDatePickeris true.As per path instructions, "Dialogs must be separated into their own composables" and "Do not embed dialogs inline within complex main screens."
🤖 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/loanChargeOff/LoanChargeOffScreen.kt` around lines 150 - 171, The inline DatePickerDialog inside LoanChargeOffForm should be extracted into a dedicated composable, matching the existing LoanChargeOffDialog pattern. Move the dialog UI and its confirm/dismiss handling into a new composable (for example, a DatePickerDialog wrapper) and have LoanChargeOffForm conditionally invoke that composable when state.showDatePicker is true. Keep the action wiring through LoanChargeOffAction.TransactionDateChanged and LoanChargeOffAction.HideDatePicker unchanged, but relocate the dialog-specific code out of the main screen composable.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.kt`:
- Around line 150-171: The inline DatePickerDialog inside LoanChargeOffForm
should be extracted into a dedicated composable, matching the existing
LoanChargeOffDialog pattern. Move the dialog UI and its confirm/dismiss handling
into a new composable (for example, a DatePickerDialog wrapper) and have
LoanChargeOffForm conditionally invoke that composable when state.showDatePicker
is true. Keep the action wiring through
LoanChargeOffAction.TransactionDateChanged and
LoanChargeOffAction.HideDatePicker unchanged, but relocate the dialog-specific
code out of the main screen composable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67b8ca79-070e-4b54-b629-4ddf34359663
📒 Files selected for processing (30)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/ChargeOffReasonOptionMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/LoanChargeOffInputMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanChargeOffRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanChargeOffRepositoryImpl.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/di/UseCaseModule.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/GetLoanChargeOffTemplateUseCase.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/LoanChargeOffUseCase.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/ChargeOffReasonOption.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/LoanChargeOffInput.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffRequestDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffResponseDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffTemplateDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktcore/ui/src/commonMain/composeResources/drawable/charge_off.xmlfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionItem.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/payments/LoanActionPaymentsScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffState.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
✅ Files skipped from review due to trivial changes (4)
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/ChargeOffReasonOption.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.kt
- feature/loan/src/commonMain/composeResources/values/strings.xml
- core/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffTemplateDto.kt
🚧 Files skipped from review as they are similar to previous changes (20)
- core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/GetLoanChargeOffTemplateUseCase.kt
- core/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffRequestDto.kt
- core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/loanChargeOff/LoanChargeOffUseCase.kt
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/LoanChargeOffInput.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanChargeOffRepositoryImpl.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.kt
- core/domain/src/commonMain/kotlin/com/mifos/core/domain/di/UseCaseModule.kt
- core/network/src/commonMain/kotlin/com/mifos/core/network/dto/loans/LoanChargeOffResponseDto.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanChargeOffRepository.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/ChargeOffReasonOptionMapper.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.kt
- core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.kt
- core/ui/src/commonMain/composeResources/drawable/charge_off.xml
- core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/mappers/loan/LoanChargeOffInputMapper.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffState.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffViewModel.kt
- core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: PR Checks / Static Analysis Check
🧰 Additional context used
📓 Path-based instructions (2)
**/*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/loanAccountAction/LoanAccountActionScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/payments/LoanActionPaymentsScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.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:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/payments/LoanActionPaymentsScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionItem.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:
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/payments/LoanActionPaymentsScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionItem.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/loanAccountAction/LoanAccountActionScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/payments/LoanActionPaymentsScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionItem.kt
🔇 Additional comments (8)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreen.kt (3)
65-126: LGTM!
172-236: LGTM! Form fields correctly source formatted date text (state.transactionDateText) from state rather than formatting in the UI layer, and use design tokens for spacing.
238-251: LGTM! Dialog correctly isolated into its own composable, driven by state.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanChargeOff/LoanChargeOffScreenRoute.kt (1)
1-35: LGTM! Route is properly@Serializable, and the past concern about passingnavControllerintoLoanChargeOffScreenis resolved — onlyonNavigateBack/onChargeOffSuccessare passed now.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionItem.kt (1)
156-489: LGTM!idfield addition is consistent across all action entries and each id appears unique, enabling thekey = { it.id }fix used downstream.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionNavigation.kt (1)
22-101: LGTM!navigateToChargeOffwiring and the newreloadLoanAccountActionScreen(pop-up-to +launchSingleTop) look correct for refreshing the action list post charge-off.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/LoanAccountActionScreen.kt (1)
113-117: LGTM! Usingit.idas the stable list key is a solid improvement overit.title.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountAction/payments/LoanActionPaymentsScreen.kt (1)
92-94: LGTM! Consistent with the sameit.idkey fix applied inLoanAccountActionScreen.kt.
sahilshivekar
changed the title
revanthkumarJ
requested changes
Jul 2, 2026
Contributor
There was a problem hiding this comment.
while you called api show a overlay loader @sahilshivekar
|
Contributor
Author
@revanthkumarJ Done now |
revanthkumarJ
approved these changes
Jul 2, 2026
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-#690
charge_off_not_implemented.mp4
charge_off_success.mp4
Note:
Loan Account Action Screen is reloaded because if the charge-off operation is successful then it needs to show Undo Charge-off instead of Charge-off
In Loan Account Action Item property with name "id" is added because StringResource (item.title) is not supported as a key for lazy column on android platform. It throws exception ->
java.lang.IllegalArgumentException: Type of the key org.jetbrains.compose.resources.StringResource@3aecd81e is not supported. On Android you can only use types which can be stored inside the Bundle.