feature(standingInstructions) : Implemented new module and network layer implementation for the standing instructions endpoint. - #2694
Open
techsavvy185 wants to merge 17 commits into
Conversation
…yer implementation for the standing instructions endpoint.
|
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:
WalkthroughThis PR adds a complete "Standing Instructions" feature enabling users to view, edit, and delete recurring payment instructions linked to client accounts. The implementation spans domain and DTO models, network service integration with Ktorfit, repository layer with mappers, MVVM state management with Compose UI, navigation routing, and seamless savings account screen integration with localized string resources. ChangesFeature implementation layers (click to expand)Standing Instructions Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant SavingsUI as Savings Screen
participant NavCtrl as NavController
participant ViewModel
participant Repo as Repository
participant DataMgr as DataManager
participant Service as HTTP Service
User->>SavingsUI: Tap "View Standing Instructions"
SavingsUI->>NavCtrl: navigate to instructions screen
NavCtrl->>ViewModel: init with route args
activate ViewModel
ViewModel->>Repo: getStandingInstructionList()
activate Repo
Repo->>DataMgr: retrieveListStandingInstructions()
activate DataMgr
DataMgr->>Service: GET standinginstructions
activate Service
Service-->>DataMgr: Flow of DTOs
deactivate Service
DataMgr-->>Repo: Flow of DTOs
deactivate DataMgr
Repo->>Repo: map to domain
Repo-->>ViewModel: DataState.Success
deactivate Repo
ViewModel->>ViewModel: build table data
ViewModel-->>SavingsUI: emit state
deactivate ViewModel
SavingsUI->>SavingsUI: render table
User->>SavingsUI: click Edit row
SavingsUI->>ViewModel: OnEditClick action
activate ViewModel
ViewModel->>ViewModel: show edit dialog
ViewModel-->>SavingsUI: emit state
deactivate ViewModel
User->>SavingsUI: confirm edit
SavingsUI->>ViewModel: OnConfirmEdit action
activate ViewModel
ViewModel->>ViewModel: set loading
ViewModel->>Repo: updateStandingInstruction()
activate Repo
Repo->>DataMgr: updateStandingInstruction()
activate DataMgr
DataMgr->>Service: PUT standinginstructions
activate Service
Service-->>DataMgr: response
deactivate Service
DataMgr-->>Repo: response
deactivate DataMgr
Repo-->>ViewModel: DataState.Success
deactivate Repo
ViewModel->>ViewModel: reload list
ViewModel-->>SavingsUI: emit state
deactivate ViewModel
SavingsUI->>SavingsUI: refresh table
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 |
techsavvy185
marked this pull request as ready for review
June 10, 2026 05:35
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.kt (1)
206-209: ⚡ Quick winMifosEmptyCard
msgandtitleare set to the same string resource.Both parameters use
Res.string.feature_standing_instructions_no_standing_instructions_found_message. Typically, a title and message should convey different information (e.g., title: "No Standing Instructions", message: "No standing instructions found for this account"). Consider using separate string resources or omitting one parameter.🤖 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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.kt` around lines 206 - 209, MifosEmptyCard is being passed the same string for both msg and title; update the call in ViewStandingInstructionsScreen to use distinct text for title and message by replacing the title argument with an appropriate title resource (e.g., Res.string.feature_standing_instructions_no_standing_instructions_found_title) via stringResource(...) or remove the title parameter if a title is not required; ensure the change targets the MifosEmptyCard(...) invocation so msg and title convey different information.
🤖 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/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstruction.kt`:
- Around line 14-15: The data classes StandingInstruction,
StandingInstructionAccount, and StandingInstructionClient are annotated with
`@Serializable` which couples the core model to networking; remove the
`@Serializable` annotations from these classes so they remain pure domain models,
and if serialization is needed for DTOs or network layers create separate
serializable DTOs or mapper functions to convert between
StandingInstruction/StandingInstructionAccount/StandingInstructionClient and
their serializable counterparts; update any call sites that relied on direct
serialization to use the new DTOs or mappers.
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.kt`:
- Around line 19-40: Update the KDoc for
StandingInstructionService.getStandingInstructions to document all parameters
and the return type: add descriptions for clientId, clientName, fromAccountId,
fromAccountType, locale, dateFormat, limit, and offset, and note the returned
Flow<Page<StandingInstructionDto>> and possible responses (200 OK); ensure each
`@param` tag matches the function parameter names exactly and briefly describes
purpose and type, and keep existing general description of the method.
In
`@feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.kt`:
- Around line 144-154: The onViewStandingInstructions lambda currently falls
back to 0L for a null savingsAccountWithAssociations.clientId and passes a magic
number 1 for account type; update the handler in SavingsAccountSummaryScreen so
that when uiState is SavingsAccountSummaryUiState.ShowSavingAccount you
explicitly guard against a null clientId (e.g., skip navigation or show an
error/placeholder) instead of using 0L, and replace the hardcoded 1 with a named
constant or enum (e.g., ACCOUNT_TYPE_SAVINGS or AccountType.SAVINGS) when
calling onViewStandingInstructions with accountId.toLong().
In
`@feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.kt`:
- Line 122: The fallback string for validity is inconsistent: replace the
trailing-space variant "-- " with the standard "--" wherever validity is
assigned (in ViewStandingInstructionsViewModel, specifically the mapping that
sets validity = instruction.validFrom ?: "-- "), so it matches the other fields
(lines using "--") and avoids uneven column alignment or visual artifacts.
- Around line 75-110: The collect block in repository.getStandingInstructionList
currently calls mutableStateFlow.update directly for DataState.Error, Loading,
and Success; instead create internal actions (e.g.,
StandingInstructionsLoadError(dataState), StandingInstructionsLoading,
StandingInstructionsLoadSuccess(dataState)) and from the collect { } call only
invoke sendAction(...) with the appropriate action (use
mapToTableData(dataState.data) inside the action payload if needed), then
implement handling of these actions inside handleAction(...) to perform all
mutableStateFlow.update mutations (setting dataState, tableData, etc.) so that
repository results flow: repository → sendAction(...) → handleAction(...) →
mutableStateFlow.update.
---
Nitpick comments:
In
`@feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.kt`:
- Around line 206-209: MifosEmptyCard is being passed the same string for both
msg and title; update the call in ViewStandingInstructionsScreen to use distinct
text for title and message by replacing the title argument with an appropriate
title resource (e.g.,
Res.string.feature_standing_instructions_no_standing_instructions_found_title)
via stringResource(...) or remove the title parameter if a title is not
required; ensure the change targets the MifosEmptyCard(...) invocation so msg
and title convey different information.
🪄 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: 36011c5a-dd7b-40b2-80ad-fd0908fe30c1
📒 Files selected for processing (33)
cmp-navigation/build.gradle.ktscmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionEntityMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionsDtoMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/StandingInstructionsRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/StandingInstructionsRepositoryImp.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionAccountEntity.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionClientEntity.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionEntity.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstruction.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstructionAccount.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstructionClient.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/BaseApiManager.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerStandingInstructions.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/standingInstruction/StandingInstructionDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.ktfeature/client/build.gradle.ktsfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktfeature/savings/build.gradle.ktsfeature/savings/src/commonMain/kotlin/com/mifos/feature/savings/navigation/SavingsNavigation.ktfeature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.ktfeature/standing-instruction/.gitignorefeature/standing-instruction/build.gradle.ktsfeature/standing-instruction/src/commonMain/composeResources/values/strings.xmlfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/di/StandingInstructionsModule.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/navigation/StandingInstructionsNavigation.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsRoute.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.ktsettings.gradle.kts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🧰 Additional context used
📓 Path-based instructions (11)
**/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/standing-instruction/src/commonMain/composeResources/values/strings.xml
**/*.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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/di/StandingInstructionsModule.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/navigation/StandingInstructionsNavigation.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/standingInstruction/StandingInstructionDto.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionClientEntity.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionEntity.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstructionClient.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionAccountEntity.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/StandingInstructionsRepositoryImp.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstruction.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsRoute.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerStandingInstructions.ktfeature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionEntityMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/StandingInstructionsRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionsDtoMapper.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstructionAccount.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/BaseApiManager.ktfeature/savings/src/commonMain/kotlin/com/mifos/feature/savings/navigation/SavingsNavigation.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.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/di/DataMangerModule.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/standingInstruction/StandingInstructionDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerStandingInstructions.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/BaseApiManager.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.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/di/DataMangerModule.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/standingInstruction/StandingInstructionDto.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionClientEntity.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionEntity.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstructionClient.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionAccountEntity.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/StandingInstructionsRepositoryImp.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstruction.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerStandingInstructions.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionEntityMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/StandingInstructionsRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionsDtoMapper.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstructionAccount.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/BaseApiManager.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.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/standingInstruction/StandingInstructionDto.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/standingInstructions/StandingInstructionClient.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstruction.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstructionAccount.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/StandingInstructionsRepositoryImp.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/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.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/standingInstructions/StandingInstructionEntityMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionsDtoMapper.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/StandingInstructionsRepository.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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/di/StandingInstructionsModule.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/navigation/StandingInstructionsNavigation.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/standingInstruction/StandingInstructionDto.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionClientEntity.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionEntity.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstructionClient.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionAccountEntity.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/StandingInstructionsRepositoryImp.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstruction.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsRoute.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerStandingInstructions.ktfeature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionEntityMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/StandingInstructionsRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionsDtoMapper.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstructionAccount.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/BaseApiManager.ktfeature/savings/src/commonMain/kotlin/com/mifos/feature/savings/navigation/SavingsNavigation.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/di/StandingInstructionsModule.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/navigation/StandingInstructionsNavigation.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/standingInstruction/StandingInstructionDto.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionClientEntity.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionEntity.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstructionClient.ktcore/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionAccountEntity.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/StandingInstructionsRepositoryImp.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstruction.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsRoute.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerStandingInstructions.ktfeature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionEntityMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/StandingInstructionsRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionsDtoMapper.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstructionAccount.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/BaseApiManager.ktfeature/savings/src/commonMain/kotlin/com/mifos/feature/savings/navigation/SavingsNavigation.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.kt
📚 Learning: 2026-02-16T08:37:28.351Z
Learnt from: kartikey004
Repo: openMF/android-client PR: 2610
File: feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersList/ClientIdentifiersListScreen.kt:60-60
Timestamp: 2026-02-16T08:37:28.351Z
Learning: In Kotlin files under the android-client module, replace MaterialTheme references with the established KptTheme import: import template.core.base.designsystem.theme.KptTheme. This should be applied consistently across files where KptTheme is used, replacing any MaterialTheme imports with the correct KptTheme import path.
Applied to files:
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt
📚 Learning: 2026-02-13T08:12:37.519Z
Learnt from: amanna13
Repo: openMF/android-client PR: 2604
File: feature/groups/build.gradle.kts:42-42
Timestamp: 2026-02-13T08:12:37.519Z
Learning: In Android projects using Jetpack Compose icons, include androidx.compose.material:material-icons-core whenever you import androidx.compose.material.icons.* (Icons.Default, Icons.Filled, Icons.Outlined, Icons.Rounded). Only add androidx.compose.material:material-icons-extended if you actually use extended icons, since it increases artifact size (~36MB) and brings in material-icons-core transitively. Apply this guideline to all Gradle Kotlin scripts (*.gradle.kts) across modules to ensure dependency scope is minimized.
Applied to files:
feature/standing-instruction/build.gradle.ktsfeature/savings/build.gradle.ktscmp-navigation/build.gradle.ktsfeature/client/build.gradle.ktssettings.gradle.kts
🔇 Additional comments (31)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.kt (1)
72-73: LGTM!Also applies to: 143-144, 214-216
cmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.kt (1)
41-42: LGTM!Also applies to: 98-99
feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/di/StandingInstructionsModule.kt (1)
12-18: LGTM!cmp-navigation/build.gradle.kts (1)
54-55: LGTM!feature/client/build.gradle.kts (1)
34-35: LGTM!feature/savings/build.gradle.kts (1)
28-29: LGTM!feature/standing-instruction/build.gradle.kts (1)
10-13: LGTM!Also applies to: 15-17, 19-36
feature/standing-instruction/.gitignore (1)
1-1: LGTM!feature/standing-instruction/src/commonMain/composeResources/values/strings.xml (1)
12-20: LGTM!feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
114-115: LGTM!Also applies to: 406-411
feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.kt (1)
40-44: LGTM!Also applies to: 46-62, 64-68
feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.kt (3)
66-89: LGTM!Also applies to: 92-136
139-155: LGTM!
216-251: LGTM!Also applies to: 254-287
feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsRoute.kt (1)
18-50: LGTM!feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/navigation/StandingInstructionsNavigation.kt (1)
16-24: LGTM!feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/navigation/SavingsNavigation.kt (1)
34-34: LGTM!Also applies to: 69-69, 113-113, 129-129
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.kt (2)
164-164: LGTM!Also applies to: 205-211
700-700: LGTM!core/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionEntity.kt (1)
18-43: LGTM!core/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionClientEntity.kt (1)
16-21: LGTM!core/database/src/commonMain/kotlin/com/mifos/room/entities/standingInstructions/StandingInstructionAccountEntity.kt (1)
16-22: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/dto/standingInstruction/StandingInstructionDto.kt (1)
14-73: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionsDtoMapper.kt (1)
19-44: 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/BaseApiManager.kt (1)
47-47: LGTM!Also applies to: 68-68, 98-98
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerStandingInstructions.kt (1)
17-41: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.kt (1)
34-34: LGTM!Also applies to: 61-61
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/StandingInstructionsRepository.kt (1)
17-26: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/StandingInstructionsRepositoryImp.kt (1)
26-59: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionEntityMapper.kt (1)
20-63: LGTM!
sahilshivekar
suggested changes
Jun 11, 2026
Contributor
|
@techsavvy185 Why row level actions are not implemented ? |
Member
Author
|
@sahilshivekar Row level actions are not implemented since there is no implementation for edit or delete for standing instructions in the app as of yet, and the jira ticket only specified creating the View SI screen as a requirement. |
Contributor
@techsavvy185 I see it's mentioned in the ticket to implement row level actions. Also there is a API on swagger UI for it. |
Member
Author
|
@sahilshivekar Oh, I missed that one. I will implement those and push the commits asap. |
Contributor
|
@techsavvy185 Please address the CodeRabbitAI suggestions relevant to your PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.kt (1)
38-48:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUse a one-shot network contract for this endpoint.
getStandingInstructionsis modeled asFlow, but this endpoint is a paged request/response call and should be a one-shotsuspendAPI returning rawPage<StandingInstructionDto>. Keeping it asFlowforces downstream repository contracts into streaming shapes unnecessarily.As per coding guidelines: in
**/network/**/*.kt, one-shot API calls must usesuspendand must not useFlow.🤖 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/services/StandingInstructionService.kt` around lines 38 - 48, The getStandingInstructions method in StandingInstructionService is currently returning Flow<Page<StandingInstructionDto>> as a one-shot paged endpoint, but per the network module coding guidelines, one-shot API calls must use suspend functions and not Flow. Change the return type from Flow<Page<StandingInstructionDto>> to Page<StandingInstructionDto> and add the suspend keyword before the function signature in the getStandingInstructions method declaration.Source: Coding guidelines
🧹 Nitpick comments (1)
feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.kt (1)
279-283: ⚡ Quick winScope table inputs to reduce recompositions in ViewStandingInstructionsScreen.kt.
ViewStandingInstructionsDatatakes the wholedialogState(Line 281), so edits in the dialog can recompose the entire table. Pass only a minimal value (e.g., selected options row id) needed for dropdown visibility.As per coding guidelines: “Avoid unnecessary recompositions in Compose.”
Also applies to: 324-332
🤖 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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.kt` around lines 279 - 283, The ViewStandingInstructionsData composable function accepts the entire dialogState parameter, which causes the table to recompose unnecessarily whenever any part of the dialog state changes. Replace the dialogState parameter with a more granular parameter that only includes the minimal information needed for the dropdown visibility (such as a selected row id). Update the function signature to remove the dialogState parameter and add the new minimal parameter, then update all call sites of ViewStandingInstructionsData (including the one around line 324-332) to pass only the required minimal value instead of the entire dialogState object.Source: Coding guidelines
🤖 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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.kt`:
- Around line 76-83: The OnEditClick action handler in
ViewStandingInstructionsViewModel is initializing edit fields with display
placeholder values (such as "-- " for validity) instead of actual data values.
These placeholders can then flow into the updateStandingInstruction API call,
sending invalid data to the backend. To fix this, separate the display values
from the actual data values: use real data fields (not display placeholders like
"-- ") when initializing the Edit dialog state in the OnEditClick handler, and
add validation in the updateStandingInstruction method to reject or sanitize
placeholder values before sending them to the API.
---
Outside diff comments:
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.kt`:
- Around line 38-48: The getStandingInstructions method in
StandingInstructionService is currently returning
Flow<Page<StandingInstructionDto>> as a one-shot paged endpoint, but per the
network module coding guidelines, one-shot API calls must use suspend functions
and not Flow. Change the return type from Flow<Page<StandingInstructionDto>> to
Page<StandingInstructionDto> and add the suspend keyword before the function
signature in the getStandingInstructions method declaration.
---
Nitpick comments:
In
`@feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.kt`:
- Around line 279-283: The ViewStandingInstructionsData composable function
accepts the entire dialogState parameter, which causes the table to recompose
unnecessarily whenever any part of the dialog state changes. Replace the
dialogState parameter with a more granular parameter that only includes the
minimal information needed for the dropdown visibility (such as a selected row
id). Update the function signature to remove the dialogState parameter and add
the new minimal parameter, then update all call sites of
ViewStandingInstructionsData (including the one around line 324-332) to pass
only the required minimal value instead of the entire dialogState object.
🪄 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: 52a86d06-7756-4e1c-9ab7-4607ad8b1561
📒 Files selected for processing (13)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionsDtoMapper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/StandingInstructionsRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/StandingInstructionsRepositoryImp.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstructionUpdate.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerStandingInstructions.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/standingInstruction/StandingInstructionUpdateResponseDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/dto/standingInstruction/UpdateStandingInstructionDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.ktfeature/standing-instruction/src/commonMain/composeResources/values/strings.xmlfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/di/StandingInstructionsModule.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.kt
✅ Files skipped from review due to trivial changes (3)
- feature/standing-instruction/src/commonMain/composeResources/values/strings.xml
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/standingInstructions/StandingInstructionUpdate.kt
- core/network/src/commonMain/kotlin/com/mifos/core/network/dto/standingInstruction/StandingInstructionUpdateResponseDto.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/di/StandingInstructionsModule.kt
- cmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/mappers/standingInstructions/StandingInstructionsDtoMapper.kt
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🧰 Additional context used
📓 Path-based instructions (8)
**/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/standingInstruction/UpdateStandingInstructionDto.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerStandingInstructions.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/standingInstruction/UpdateStandingInstructionDto.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/dto/standingInstruction/UpdateStandingInstructionDto.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/StandingInstructionsRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerStandingInstructions.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/StandingInstructionsRepositoryImp.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/dto/standingInstruction/UpdateStandingInstructionDto.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/StandingInstructionsRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerStandingInstructions.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/StandingInstructionsRepositoryImp.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.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/StandingInstructionsRepository.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/StandingInstructionsRepositoryImp.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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.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/dto/standingInstruction/UpdateStandingInstructionDto.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/StandingInstructionsRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerStandingInstructions.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/StandingInstructionsRepositoryImp.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.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/dto/standingInstruction/UpdateStandingInstructionDto.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/StandingInstructionsRepository.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/StandingInstructionService.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerStandingInstructions.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/StandingInstructionsRepositoryImp.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsScreen.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.kt
🔇 Additional comments (3)
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerStandingInstructions.kt (1)
22-61: LGTM!core/network/src/commonMain/kotlin/com/mifos/core/network/dto/standingInstruction/UpdateStandingInstructionDto.kt (1)
14-30: LGTM!core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/StandingInstructionsRepositoryImp.kt (1)
63-89: LGTM!
Comment on lines
+76
to
+83
| is ViewStandingInstructionsAction.OnEditClick -> { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| dialogState = ViewStandingInstructionsState.DialogState.Edit( | ||
| rowData = action.rowData, | ||
| amount = action.rowData.amount, | ||
| validFrom = action.rowData.validity, | ||
| beneficiary = action.rowData.beneficiary, |
There was a problem hiding this comment.
Display placeholders are leaking into update payloads in ViewStandingInstructionsViewModel.kt.
OnEditClick initializes edit fields from table row display values (Line 81-82), and validity fallback is "-- " (Line 263). That value can flow into updateStandingInstruction(...) (Line 171-179), producing invalid API data when users confirm without correcting placeholders.
Also applies to: 171-179, 263-263
🤖 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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.kt`
around lines 76 - 83, The OnEditClick action handler in
ViewStandingInstructionsViewModel is initializing edit fields with display
placeholder values (such as "-- " for validity) instead of actual data values.
These placeholders can then flow into the updateStandingInstruction API call,
sending invalid data to the backend. To fix this, separate the display values
from the actual data values: use real data fields (not display placeholders like
"-- ") when initializing the Edit dialog state in the OnEditClick handler, and
add validation in the updateStandingInstruction method to reject or sanitize
placeholder values before sending them to the API.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.kt (1)
98-104:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential exception when parsing amount validation.
Line 99 calls
action.value.toDoubleOrNull()to check for null, but then immediately callsaction.value.toDouble()which can throwNumberFormatExceptionif the value is not a valid double. This creates a redundant and potentially unsafe check.🔧 Simplified validation fix
EditField.AMOUNT -> { - val amountError = if (action.value.toDoubleOrNull() == null || action.value.toDouble() <= 0) { + val parsedAmount = action.value.toDoubleOrNull() + val amountError = if (parsedAmount == null || parsedAmount <= 0) { "Invalid amount" } else { null } currentEdit.copy(amount = action.value, amountError = amountError) }🤖 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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.kt` around lines 98 - 104, The amount validation in the EditField.AMOUNT case has a potential NumberFormatException issue. Currently, the code calls action.value.toDoubleOrNull() to check for null, then calls action.value.toDouble() again which can throw an exception if the value is invalid. Refactor this by calling toDoubleOrNull() once and storing the result in a variable, then use that single variable to check both for null and whether the parsed value is greater than 0. This eliminates the redundant conversion and prevents the potential exception when creating the amountError in the currentEdit.copy() call.
🤖 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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.kt`:
- Around line 264-272: The CurrencyFormatter.format() call in the
ViewStandingInstructionsViewModel is receiving a nullable Double value
(instruction.amount) without null checking when state.currencyCode is not empty.
Add an explicit null check for instruction.amount before passing it to
CurrencyFormatter.format(), and return "--" as the fallback value if
instruction.amount is null, similar to the else branch handling.
---
Outside diff comments:
In
`@feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.kt`:
- Around line 98-104: The amount validation in the EditField.AMOUNT case has a
potential NumberFormatException issue. Currently, the code calls
action.value.toDoubleOrNull() to check for null, then calls
action.value.toDouble() again which can throw an exception if the value is
invalid. Refactor this by calling toDoubleOrNull() once and storing the result
in a variable, then use that single variable to check both for null and whether
the parsed value is greater than 0. This eliminates the redundant conversion and
prevents the potential exception when creating the amountError in the
currentEdit.copy() call.
🪄 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: a5a101cc-014e-4ca9-9334-328680607435
📒 Files selected for processing (4)
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/navigation/SavingsNavigation.ktfeature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsRoute.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/navigation/SavingsNavigation.kt
- feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountSummary/SavingsAccountSummaryScreen.kt
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsRoute.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsRoute.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.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/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsRoute.ktfeature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.kt
🔇 Additional comments (5)
feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsViewModel.kt (4)
273-273: Trailing space in validity fallback is still present.This was flagged in a previous review - the
"-- "fallback has a trailing space inconsistent with other fields using"--".
78-91: Edit dialog initialization from display values was previously flagged.The past review comment correctly identified that initializing edit fields from formatted display values (like
amountandvalidity) can leak placeholders into API payloads.
218-253: MVI reducer pattern violation was previously flagged.Direct state mutations inside
collect {}block violates the MVI architecture guidelines. The past review comment provides a comprehensive refactor to dispatch internal actions instead.
280-306: LGTM!feature/standing-instruction/src/commonMain/kotlin/com/mifos/feature/standingInstruction/viewStandingInstructions/ViewStandingInstructionsRoute.kt (1)
18-43: LGTM!
Contributor
|
@techsavvy185 It's showing 500 amount even though you edited it to 5005 |
Member
Author
|
@sahilshivekar It's because I populated this list manually to show how it is going to look like. While the edit functionality is pinging to change the value using the API, for which the account does not exist obviously. |
Member
Author
|
And, I've used MifosEmptyCard for the current empty state. |
Contributor
Understood. But it should be tested with real account for which SI exists, if there is no such account then try to create one please (you can take help from victor for it). |
Member
Author
|
@sahilshivekar well that's the issue with this functionality, it is not really working and @biplab1 was following up on this i believe. |
Collaborator
|
@sahilshivekar @biplab1 @revanthkumarJ could you guys review this PR |
Contributor
|
@techsavvy185 are the apis working ?. |
Member
Author
|
@revanthkumarJ The fetch and update APIs by themselves are functional, however the create API, I've tested it a few times using various dummy data to test my portion, but it is not working, also the web app create SI part is broken as I have highlighted previously as well. I am sorry for the delayed response, I have been busy with some stuff. Screen.Recording.2026-07-26.at.1.14.18.AM.mov |
…oanStandingInstructions
|
👋 Hi @techsavvy185 — thank you for your pull request. This PR is currently blocked because we do not have a Contributor License Agreement (CLA) on file for your GitHub account. To get unblocked:
|
mifos-cla-check
Bot
added
the
cla-required
|
👋 Hi @techsavvy185 — thank you for your pull request. This PR is currently blocked because we do not have a Contributor License Agreement (CLA) on file for your GitHub account. To get unblocked:
|
techsavvy185
marked this pull request as draft
July 26, 2026 18:14
techsavvy185
marked this pull request as ready for review
July 26, 2026 18:14
techsavvy185
marked this pull request as draft
July 27, 2026 06:53
techsavvy185
marked this pull request as ready for review
July 27, 2026 06:53
…oanStandingInstructions
mifos-cla-check
Bot
removed
the
cla-required
|
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
5 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-#672
With Empty response in case no SI exist:
Screen_recording_20260610_104525.webm
WIth Mock Data:(The delete isn't working since the data is populated locally)
Screen_recording_20260620_001943.webm
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.